[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-bugs-dist
Subject:    [valgrind] [Bug 371916] execution tree xtree concept
From:       Philippe Waroquiers <bugzilla_noreply () kde ! org>
Date:       2016-11-11 13:18:58
Message-ID: bug-371916-17878-snVCROfhCv () http ! bugs ! kde ! org/
[Download RAW message or body]

https://bugs.kde.org/show_bug.cgi?id=371916

--- Comment #21 from Philippe Waroquiers <philippe.waroquiers@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.=
[prev in list] [next in list] [prev in thread] [next in thread]