https://bugs.kde.org/show_bug.cgi?id=367995
--- Comment #11 from Ruurd Beerstra <ruurd.beers...@infor.com> --- Hi, Answers between the text below... >-----Original Message----- >From: Julian Seward via KDE Bugzilla [mailto:bugzilla_nore...@kde.org] >Sent: Tuesday, September 20, 2016 12:47 > >https://bugs.kde.org/show_bug.cgi?id=367995 > >--- Comment #9 from Julian Seward <jsew...@acm.org> --- Ivo, thank you for >reviewing this; Ruurd, >thank you for addressing Ivo's comments. > >I looked at the revised patch. I am generally a bit nervous about mempool >changes given that they >are not much used and I am not sure any of the Valgrind developers really >understands that code >any more. >But given that it looks plausible and it has some tests, I am OK with it, >provided the issues below >can be cleared up. > >I have some comments/questions: > > >(1) >When the new functionality is not in use (that is, neither MEMPOOL_AUTO_FREE >nor MEMPOOL_METAPOOL >has been passed to the mempool creation routines), is there any weakening of >the sanity checking >or assertions, relative to pre-patch? AFAIK: No. I tried my best to make sure I did nothing to break any old behavior. When both flags are off, you get a Plain Old Memory pool. The regression tests that deal with pools all still succeed. >(2) >--- include/valgrind.h (revision 15958) >+++ include/valgrind.h (working copy) >+#define MEMPOOL_AUTO_FREE 1 >+#define MEMPOOL_METAPOOL 2 > >(2a) Please call these VALGRIND_MEMPOOL_{AUTO_FREE,METAPOOL} so as > to be consistent with the rest of the file and so as to reduce > the chances of weird namespace clashes, given that this file is > #include-d into arbitrary end-user code. And update the doc diff > accordingly. Done. All occurrences in all files updated. >(2b) Please mention, in the comment, that the flags are intended to be > ORd together (viz, it's not an enum). This wasn't obvious to me > on first reading. Done. >(3) >=================================================================== >--- memcheck/mc_include.h (revision 15958) >+++ memcheck/mc_include.h (working copy) >@@ -67,6 +67,7 @@ > Addr data; // Address of the actual block. > SizeT szB : (sizeof(SizeT)*8)-2; // Size requested; 30 or 62 >bits. > MC_AllocKind allockind : 2; // Which operation did the allocation. >+ Bool is_mempool_block; > ExeContext* where[0]; > /* Variable-length array. The size depends on MC_(clo_keep_stacktraces). > This array optionally stores the alloc and/or free stack trace. */ > >I am concerned about the addition of a Bool to struct _MC_Chunk. >There can be hundreds of thousands of these. Given that an extra Bool will >add another word to >the structure, the new Bool could increase memory use by several megabytes. >There have been significant >efforts made in the past to keep these structures small, and I don't want to >undo them. > >Is it absolutely necessary to add this new Bool? Is there a way to avoid it? I see only one way: Every time the "is_mempool_block" value is used, find the status of the chunk dynamically instead. That involves scanning all memory-pool chunk-lists (mp->chunks) of all existing memory pools for the current chunk. Hmm. That was actually quite easy. I've dropped the is_mempool_block Bool, just now, and created a: Bool MC_(is_mempool_block)( MC_Chunk* mc_search ); function. It think it is reasonably efficient tradeoff. It may have to search all chunks of all memory pools to find that the chunk is (not) part of a memory pool, but that is in only in describe_addr and detect_memory_leaks functions, and both functions are only used when a problem has been detected (I think). If you don’t use custom allocators, and/or those custom allocators do not use memory pools, nothing extra happens. I've just compiled that, and it seems to work as before (regression tests all OK). Before bothering you with this updated patch I want to run a few more test but the day is ending here and I've gotta run. If it works I'll submit the patched patch :-) Idea: It may be a good idea to keep globally track of the existence of a METAPOOL memory pool, to avoid the scans because the interesting things it has to scan for only happen when such a pool currently exists. I'll sleep on that one. How much time have I got before the window closes? Thank you for your remarks, Ruurd > >-- >You are receiving this mail because: >You reported the bug. > -- You are receiving this mail because: You are watching all bug changes.
