https://bugs.kde.org/show_bug.cgi?id=371916
--- Comment #16 from Philippe Waroquiers <philippe.waroqui...@skynet.be> --- (In reply to Josef Weidendorfer from comment #15) > Hi Philippe, nice patch! > > I think it is good to move functionalty from tools to the core, if it > can make other tools better or easier to write. Attaching data to call > trees built from stack traces should fit here. > However, I did not check the large rewrite of massif. > > General comments (I may be missing something here): > > * is this bound to memory allocation, or is memory allocation just > an example use case of the API? It would be nice if tools could > attach arbitrary data to xtrees. Perhaps separate the memory allocation > stuff into different files (e.g. xtree_memalloc.h). The API is done so that arbitrary data can be attached. Of course, to be output as a callgrind or massif file, the data must be translatable in (positive) integers values. For what concerns separating the memory allocation stuff in another file: that is a good suggestion, I will take a look at this. I think this should be no problem. > > * behavior of some functions in the XTree API depend on command line > options. Wouldn't it be better for a tool using this API to be in > full control here, ie. the tool parses command line > options and pass them as flags? Basically, that is what the patch does: command line options are parsed (specifically --xtree-memory=none|allocs|full) and then the tool calls the memory support functions according to the command line option (e.g. calling VG_(XTMemory_Full_alloc)). The tool is in full control, e.g. it might implement --xtree-memory=allocs completely differently, if needed. > About the changes in the malloc > wrappers, I think it would be better if the tool can register handlers, > and these handlers then call e.g. VG_(XTMemory_Full_alloc). I guess this could be done, but I think this would be less practical than the current approach: the malloc replacement is today calling a tool replacement function. So, these replacement functions are doing the job of e.g. recording the allocated blocks in a tool specific way. In this replacement function, the tool can (if desired) call e.g. VG_(XTMemory_Full_alloc)). IMO, having another handler separated from the current handler function would complexity things, as e.g. the tool would have to associate the work/data done/prepared in the 'first handler' with the 'following' handler calls related to xtree. So, in summary, there is now a handler that tells the tool to e.g. execute a malloc, and this tool function can do the needed work in one single invocation (e.g. re-using the execontext done for tool purpose as data input for the xtree). > > About the XTree API, why do you need these add/sub variants? > Why not just have a function to get a pointer to the value to be updated? > Any reduction operation may be useful, such as min/max. Yes, any operation can be done in the add/sub : these functions get a pointer to the value being updated. It might be possible to use maybe a more generic name than add/sub (maybe Operate(...) ?), but then we will need more arguments for Operate to indicate what this has to really do, and m_xtree.c itself need to do some additions to e.g. produce some totals. So, one way or another, there is an (somewhat flexible) notion of addition which is needed. The current add/sub_data_fn are matching the current usage (xtree memory). As a follow-up, I intend to work on a 'syscall xtree' which e.g. might capture min/max (or maybe an histogram) of values. If needed, we might revisit the interface of the *_data_fn (e.g. I suspect we might need a destroy_data_fn if we want a more flexible/variable data set). What I can do in any case is to add some comments to indicate that the semantic of 'add' is quite flexible, and that e.g. a 'max' or 'min' can be computed as part of the an 'add'. -- You are receiving this mail because: You are watching all bug changes.
