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

>  One copy is necessary (it's due to the bad kvm API), but we
> can avoid the others.
> 
> In any case the data will be copied by the kernel.
> 
> >
> >>
> >>  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;
}
[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;
}
[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.


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

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