ABI vs. API compatibility
Par Benoît Dejean le mardi, 22 avril 2008, 08:39 - GNOME - Lien permanent
glibtop_get_proc_mem
libgtop has a function glibtop_get_proc_mem to retrieve basic memory usage of a process. It fills a struct glibtop_proc_mem which looks like:
struct _glibtop_proc_mem
{
guint64 flags;
guint64 size;
guint64 vsize;
guint64 resident;
guint64 share;
guint64 rss;
guint64 rss_rlim;
};
Yes, size/vsize and resident/rss look like duplicate. At least on the linux implementation, even if size/vsize and resident/rss come from /proc/self/stat and /proc/self/statm, you can see in linux/fs/proc/{array,task_mmu}.c that they have the same values. So, it seems to me that the only unique members of struct glibtop_proc_mem are size, resident and share (ok there are also flags which flags which members are filled and rss_lim).
linux proportional set size
Linux 2.6.25 comes with a new stat in /proc/self/smaps called pss which is even smarter/accurate than private_dirty. There's glibtop_get_proc_map which currently have all the smaps member but not this new pss. So what is the smarter way to get this new pss in libgtop without breaking everything ?
- break the ABI ?
I could simply extend struct glibtop_proc_map. That would break the ABI, which i'm allowed to because libgtop is desktop. But that's bad practice since packagers have to rebuild everything. That's a painful migration that may delay the adoption of newer versions of the library.
- break the API ?
What about cleaning up the glibtop_proc_mem duplicate members, mark unused some of them and rename rss to pss while keeping it binary compatible ? That would make the API a bit more sensible. I've scanned the Debian/unstable archive and that would break the build 3-6 packages but i would be able to submit trivial patches to fix glibtop_get_proc_mem usage. (And also add glibtop_init(); which are missing everywhere).
- hide pss inside rss ?
So what's best ?
Commentaires
If it's this simple then you can just add a new function that returns a new struct, and deprecate (not break) the old one.
Well, all these options are ABI breaking - the 'break the API' options also break ABI, just subtly. which is worse, as its not obvious to developers or package maintainers that something has changed.
IMHO, the best option is to deprecate glibtop_get_proc_mem and add a new function (say glibtop_get_process_usage or such) that takes a pid and an enum value for which field to return.
Looks like that duplicated guint64 could fit a pointer that could point to a new struct with more padding. However, I'm wondering what's the point of this public structure - wouldn't separate function APIs for a set of these be more ABI proof? if that's a good idea, could just not expose PSS in that struct at all and introduce some new API for this, say glibtop_proc_get_pss. Then again, I assume it's all retrieved in one go and that would reduce efficiency when one needs to call get_pss, get_rss and so on in a row?
If you need to stick with a struct, it might be a good idea to at some point introduce a new one that has some padding available for future expansion and a new function, say glibtop_get_proc_mem2 (yes, need a better name) that returns it, having library users that need PSS information convert to the API and depend on the new libgtop version.
Sorry, just thinking out loud, maybe something useful
You could start using symbol versioning, and provide both.
Well, i cannot deprecate anything because that would mean throw away all the different/untested implemtentations on all !linux arch. Adding a new function also means breaking all !linux arch until someone test it (which almost always happens after the first stable release).
Storing pointers is not an option because the struct may be marshalled across a pipe. And the marshalling code is already terribly broken.
Adding reserved members is very hard when you don't want to bloat your struct. I've already provisioned glibtop_cpu for 32 CPU, that makes sizeof ~ 2KiB.
If you decide to break the API, one option would be to add a "version" field to the start of the struct. Require the caller to fill it in before calling glibtop_get_proc_mem() so you can tell how big the caller thinks the struct is. Include the current version as a #define in the header so the caller can use it to initialise the structure to the right version.
If you need to expand the struct in the future, increment the version number and add new fields to the end. Then make glibtop_get_proc_mem() only fill in the fields if the version number is appropriate.
I gotta agree with Rob... Leave this code alone, deprecate it, and just add a new call that takes a selector to tell what value to return.
Otherwise, you'll have to figure out hack upon hack upon hack every time you want to make this sort of change in the future. And, given kernel development, it won't be long before you'll need to extend it again...
Benoît> "Well, i cannot deprecate anything because that would mean throw away all the different/untested implemtentations on all !linux arch. Adding a new function also means breaking all !linux arch until someone test it (which almost always happens after the first stable release)."
How about adding the PSS support to the existing functions, but afterwards renaming glibtop_get_proc_mem to the new name, while adding a compatibility wrapper of the old name (keeping API and ABI in effect) that constructs the old style struct out of the results it gets from the new one that has more data - should be just some struct field copying and throwing away the bigger struct before returning the copied (deprecated) compatibility one from glibtop_get_proc_mem
Whenever i add a new functionnality (which is implemented as 3/4 functions), it has to be ported on linux, the 2 BSD arch and eventually solaris, darwin or aix. I can't do all these ports because of a lack of time/interest. So adding a new function for a single guint64 is too much work/breakage.
I was looking for alternatives where i would be able to do all the work myself:
- breaking the ABI moves all the work to distro packagers
- breaking the API would let me submit trivial patches to upstreams
err, yes, you need to implement PSS in all other platforms if you want to add it and not leave it empty for non-linux. Obviously. But you don't need to implement anything else already done yet again with the suggestion in comment #8...