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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux