Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

["Michael S. Tsirkin" <mst@redhat.com>]

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