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

[Avi Kivity <avi@redhat.com>]

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).

> >  >>
> >  >>   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.

> [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).

> [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.

> >  >>    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.

-- 
error compiling committee.c: too many arguments to function