https://bugs.kde.org/show_bug.cgi?id=367995
--- Comment #13 from Ruurd Beerstra <ruurd.beers...@infor.com> ---
Hello again,
In follow up of yesterday's discussion: I've created attached metapoolv2.patch.
It addresses the remarks given by Julian.
I've managed to remove the is_mempool_block Bool from the chunk-struct.
It now dynamically determines that a chunk is a mempool block by scanning all
mempool blocks in all mempools.
Since there won’t be an enormous amounts of pools, and chunks in pools are
typically pretty big so there won’t be a huge number of those, and the scan
only happens in descr_addr and detect_memory_leaks, this has no measurable
impact on performance (at least, in my tests).
I did tests today with this version of valgrind: no problems detected. All
regression tests run OK as before.
So I am happy with this, hope you are too.
Best regards,
Ruurd
-----Original Message-----
From: Ruurd Beerstra
Sent: Tuesday, September 20, 2016 16:55
To: 'bug-cont...@bugs.kde.org' <bug-cont...@bugs.kde.org>
Subject: RE: [valgrind] [Bug 367995] Integration of memcheck with custom memory
allocator
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.
