On Sun, Nov 28, 2010 at 03:14:17PM +0200, Avi Kivity wrote:
> On 11/28/2010 01:44 PM, Michael S. Tsirkin wrote:
> >On Sun, Nov 28, 2010 at 11:54:26AM +0200, Avi Kivity wrote:
> >> On 11/28/2010 11:50 AM, Michael S. Tsirkin wrote:
> >> >> >
> >> >> >Another problem is that there seem to be two memory allocations and a
> >> >> >copy here, apparently just to simplify error handling. It might be fine
> >> >> >for this test but won't scale for when performance matters.
> >> >>
> >> >> When it matters, we can fix it. I don't see msr read/write becoming
> >> >> a hot path.
> >> >
> >> >It will be very painful to fix it.
> >>
> >> Why?
> >
> >Because the API returns a vector.
>
> Returning an object does not involve a copy (return value optimization).
Yes, but assigning the value in the code that uses it will, unless again
you do this in an initializer.
> >> >>
> >> >> The compiler should optimize it away completely.
> >> >
> >> >Should as opposed to does. Want me to try a simple test?
> >>
> >> Please.
> >
> >Just for fun: optimize for size, and compare code sizes.
> >The C++ code is yours but I have removed all use of STL to make
> >it more of an even fight. I checked by object and executable size.
> >Note that this is shared library build: a C++ executable
> >will pull in a large C++ library, a C executable won't.
> >If you are interested in an STL based example let me know.
> >You can take it from here and make it more real if you like,
> >or look at the assembler output.
> >
> >------------------------------
> >[mst@tuck ~]$ cat test.c
> >#include<sys/ioctl.h>
> >#include<sys/types.h>
> >#include<sys/stat.h>
> >#include<fcntl.h>
> >#include<unistd.h>
> >#include<errno.h>
> >
> >int main(int argc, char **argv)
> >{
> > int fd = open("/dev/kvm", O_RDWR);
> > int r;
> > if (fd< 0)
> > goto open_err;
> > r = ioctl(fd, 0, 0);
> > if (r< 0)
> > goto ioctl_err;
> > return 0;
> >ioctl_err:
> > close(fd);
> >open_err:
> > return -1;
> >}
>
>
> This code is not reusable. Everywhere you use an fd, you have to
> repeat this code.
But that's not a lot of code. And you can abstract it away at a higher
level. For example kvm_init and kvm_cleanup would setup/cleanup
state in a consistent way.
My experience tells me C++ code has much more boilerplate code that you
are forced to repeat over and over. This is especially true for unix
system programming: by the time you are done wrapping all of unix you
have created more LOC than you are ever likely to save.
> >[mst@tuck ~]$ gcc -c -Os test.c
> >[mst@tuck ~]$ size test.o
> > text data bss dec hex filename
> > 97 0 0 97 61 test.o
> >[mst@tuck ~]$ gcc -Os test.c
> >[mst@tuck ~]$ size a.out
> > text data bss dec hex filename
> > 1192 260 8 1460 5b4 a.out
> >[mst@tuck ~]$ wc -l test.c
> >22 test.c
> >------------------------------
> >[mst@tuck ~]$ cat kvmxx.cpp
> >extern "C" {
> >#include<sys/ioctl.h>
> >#include<sys/types.h>
> >#include<sys/stat.h>
> >#include<fcntl.h>
> >#include<unistd.h>
> >#include<errno.h>
> >}
> >
> >namespace kvm {
> >
> > class fd {
> > public:
> > explicit fd(const char *path, int flags);
> > ~fd() { ::close(_fd); }
> > long ioctl(unsigned nr, long arg);
> > private:
> > int _fd;
> > };
> >
> >};
> >
> >namespace kvm {
> >
> >static long check_error(long r)
> >{
> > if (r == -1) {
> > throw errno;
> > }
> > return r;
> >}
> >
> >fd::fd(const char *device_node, int flags)
> > : _fd(::open(device_node, flags))
> >{
> > check_error(_fd);
> >}
> >
> >
> >long fd::ioctl(unsigned nr, long arg)
> >{
> > return check_error(::ioctl(_fd, nr, arg));
> >}
> >
> >}
> >
> >int main(int ac, char **av)
> >{
> > try {
> > kvm::fd fd("/dev/kvm", O_RDWR);
> > fd.ioctl(0, 0);
> > } catch (...) {
> > return -1;
> > }
> > return 0;
> >}
>
> class kvm::fd is reusable, if you embed it in another object you
> don't have to worry about errors any more (as long as the object's
> methods are exception safe).
To get exception safe code, you have to constantly worry about errors.
And it's easier to spot an unhandled return code than exception-unsafe
code: gcc actually has __attribute__((warn_unused_result)) which
might help catch common errors. No such tool to catch
exception-unsafe code AFAIK.
> >[mst@tuck ~]$ g++ -c -Os kvmxx.cpp
> >[mst@tuck ~]$ size kvmxx.o
> > text data bss dec hex filename
> > 529 0 0 529 211 kvmxx.o
> >[mst@tuck ~]$ g++ -Os kvmxx.cpp
> >[mst@tuck ~]$ size a.out
> > text data bss dec hex filename
> > 2254 308 16 2578 a12 a.out
> >[mst@tuck ~]$ wc kvmxx.cpp
> >56 kvmxx.cpp
> >------------------------------
> >
> >
> >One interesting thing is that the object size grew
> >faster than linked executable size.
> >This might mean that the compiler generated some
> >dead code that the linker then threw out.
> >It's also interesting that C++ managed to use up
> >more data/bss storage.
>
> C++ will have much larger data and code sizes because it uses DWARF
> tables for unwinding and generates stack unwinding code.
> These are all out of the hot path.
I only wanted to see whether the compiler would optimize it away
completely as you said it should :). Getting a convincing proof of
whether this matters would be much harder.
> >> >> There's been a lot
> >> >> of work in gcc on that.
> >> >>
> >> >> About compile times, I don't care much.
> >> >
> >> >I do. You will too when we have codebase that can be built as fast as
> >> >we commit things, so buildbot breaks.
> >> >This is common in C++ based projects.
> >>
> >> If kvm-unit-tests.git takes to long to compile, I'll be very happy.
> >
> >If the claim is "it's so small it does not matter what it's written in"
> >then I guess don't mind. But then - why do we care about
> >error handling so much? For the test, it's probably ok to just assert
> >after each ioctl/malloc and be done with it.
>
> Yes, all the correctness is more or less pointless here. Like I
> said, this is an experiment to see what things look like. I guess
> each side will it as proving its claims.
This is exactly what seems to be happening.
I did my best to try and be objective and point out real issues,
but you probably guessed which side I am on already :).
> --
> error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
