Re: [Libguestfs] [nbdkit PATCH 3/5] api: Add nbdkit_string_intern helper

On 8/25/20 2:21 PM, Richard W.M. Jones wrote: If you allocate 100 bytes for every connection, but no longer reference the memory after .close, then intern'ing for the lifetime of the server will add up to a megabyte of usage after 10,000 guests even if most of those clients are no longer connected; while intern'ing just for the lifetime of the connection does not. It's actually pretty decent heuristics (and thus I didn't give the API a knob) - if you are allocating a string in response to .load, .config, or other callback outside of a connection context, you probably want that string to last until .unload; if you are allocating a string in response to .list_exports, .default_export, .open, or anything else in response to a given client connection, you probably don't need that string any longer than the .close of that same client, except maybe for the first client (but even then, you could use .get_ready to do that sort of global setup without waiting for the first connection). Yeah, I did think about a flag, and decided it wasn't worth it. When called outside the context of a connection, the API would fail if you request in-connection lifetime; and when called inside the context of a connection, requesting an out-of-connection lifetime gets us right back to that appearance of a memory leak if it is repeated for every connection (even though we aren't actually leaking anything, and valgrind is happy, the constant growth of memory usage as more clients come and go is annoying); and if it is not repeated for every connection, it should be just as easy to do the initialization during .get_ready instead of needing a knob. It isn't now, but the contract doesn't prevent it from doing so in the future. Callers are only promised that the lifetime is at least as long as .close, but nothing would stop us from making it longer if that made our implementation use memory more compactly. But right now, I don't even bother hashing or searching for duplicates, but always append at the end. Performance wise, it may mean a bit more memory usage and/or cleanup time, but so far there were few enough uses that the main benefit (of a lifetime guaranteed by nbdkit, with automatic cleanup without bookkeeping in the plugin) seemed to outweigh the need for more complexity. Or even a memdup. Yeah, those are possibilities as well. Doing all three (strdup, strndup, memdup) up front isn't hard. If API proliferation is a problem, you can combine strdup and strndup by accepting -1 as a length to mean stop at NUL termination; but I'm less worried about API proliferation and more about ease-of-use. The nice part of the API as proposed is that a pool allocator for intern'd strings is still viable! Well, we have to be careful that a realloc doesn't invalidate prior return values, but nothing is requiring us to do a fresh malloc() per intern, rather than just carving out a slice of a larger pool when room remains and only malloc'ing large chunks (maybe 64k) when room is not available; where cleanup at the end of a connection or server lifetime only has to free the chunks instead of individual intern'd strings within those chunks. In contrast, if we had instead proposed an API where the user transfers ownership of a malloc()d string into the server, and the server is now responsible for calling free(), is much more constrained (we can't change that interface without breaking existing clients). Thus, avoiding a hard dependence on malloc() in our ABI is a good thing. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org