https://bugs.kde.org/show_bug.cgi?id=371916
--- Comment #21 from Philippe Waroquiers <philippe.waroqui...@skynet.be> ---
(In reply to Julian Seward from comment #18)
> * I assume there are no regressions with correctness or performance
> with this, yes? Could you do a self-host Memcheck run at some point?
Massif functional behaviour is unchanged (with the exception of some inversion
when 2 stacktraces have allocated the same amount: the old and new
implementation
do not output such 'equal consumers' in the same order.
Massif performance is improved (in CPU and memory).
For other tools (memcheck, helgrind) : if the xtree feature is not used,
the impact is one additional 'if' condition
in the malloc/free interception code.
>
> * The large size of the patch concerns me a bit. I would be happier
> if it could be split into two parts:
>
> (1) that creates m_xtree.c and refactors Massif to use it, but
> does not change the user-visible functionality at all. So
> it is an implementation-only change, and
>
> (2) a patch that builds on (1), that supplies the new functionality.
>
> Is that possible, if it is not a lot of work?
Ok, I will (try to) commit in some (smaller) pieces (which will I hope give
a buildable V at each commit).
>
>
> [1] Small comments:
>
> ----------------
>
> +typedef
> + struct { Addr a; const HChar* sym_name; PtrdiffT offset; }
> + Sym_Name_CacheEnt;
>
> I prefer to have something more descriptive than just "a" for the
> first field.
Renamed to sym_avma, as suggested/
>
> ----------------
>
> +" --xtree-memory=none|allocs|full profile heap memory in an xtree
> [none]\n"
> +" and produces a report at the end of the
> execution\n"
> +" none: no profiling, allocs: current
> allocated\n"
> +" size/blocks, full: profile current and
> cumulative\n"
> +" allocated size/blocks and freed
> size/blocks.\n"
> +" --xtree-memory-file=<file> xtree memory report file
> [xtmemory.kcg.%%p]\n"
>
> This flag only has effect for tools that replace malloc, correct? Is
> it listed in the section "user options for Valgrind tools that replace
> malloc:" ?
Yes, it is listed there in -help output, and documented in the user manual
around malloc/free related arguments.
>
> ----------------
>
> + This file is part of Valgrind, a dynamic binary instrumentation
> + framework.
> +
> + Copyright (C) 2016-2016 Philippe Waroquiers
>
> For m_xtree.c, if there is a lot of code in there which has been moved
> from massif and/or callgrind, and is not much changed, I think it
> would be diplomatic to add a line or two explaining that the original
> authors were Nick and/or Josef. See the top of coregrind/m_wordfm.c for
> an example.
m_xtree.c is a completely new implementation of the Xtree (I have recuperated
a few lines for the production of the massif header).
In any case, I have added a paragraph to mention that the xtree initial idea
was in massif developed by Nick.
>
> ----------------
>
> // growing such a block, but for consistency (it also simplifies things) we
> // ignore such reallocs as well.
> +// XTREE??? why can't we just consider that a realloc of an ignored
> +// alloc is just a new alloc (i.e. do not remove the old sz from the stats).
>
> and again later. The "XTREE???" is confusing -- I don't know what it
> signifies. Can you maybe write instead something like "PW Nov 2016,
> xtree work:"? I do that in comments from time to time.
Updated as suggested.
>
> ----------------
>
> Per previous comments in the bug re CamelCase vs snake_case, I really
> have no problem mixing them. I like to use camelcase, with a capital
> first letter for type names, and snake case when function names get
> long. But no fixed rules.
Yes, there is no fixed rule.
The comment of Ivo was in any case handled, as there was really too much
name variation in the API.
--
You are receiving this mail because:
You are watching all bug changes.
