Support custom sub-component reporting in the memory metric |
||
Issue description[This is a generalization of crbug.com/628239 ] Currently the memory metric reports the top-level components and few hard-coded sub-components (like v8/malloc). It would be great if we could specify sub-components for reporting in a generic way. This would allow us to have more V8 specific memory stats in V8 benchmarks without spamming the system health benchmarks. We propose to extend the tr.metrics.sh.memoryMetric function with an additional parameter that specifies sub-components we are interested in. For example consider a V8 memory dump. It has the following structure: -v8 - isolate_0x8710d868000 - heap_spaces - code_space - large_object_space - map_space - new_space - old_space - other_spaces - malloc The new parameter for the memoryMetric function could look like: OPTION 1: tr.metrics.sh.memoryMetric(values, model, { componentsOfInterest: [ { component: 'v8', levels: 3 } ] }); This would report 3 levels of v8 memory dump: v8, v8:isolate, v8:isolate:heap_spaces, v8:isolate:malloc (it is not yet clear how to handle _0x8710d868000 suffix of isolate). Alternatively, we could use path selectors to specify components for reporting: OPTION 2: tr.metrics.sh.memoryMetric(values, model, { componentsOfInterest: [ { selector: ['v8'] }, { selector: ['v8', /(isolate)_.*/, /(heap)_spaces/] }, { selector: ['v8', /(isolate)_.*/, /(heap)_spaces/, 'code_space'] }, { selector: ['v8', /(isolate)_.*/, 'malloc'] } ] }); Each path is an array of (string|regex). A string element requires exact match at that position. Each regex has one capture group, which is used for reporting (this solves the isolate suffix problem). So the example would report v8, v8:isolate:heap, v8:isolate:heap:code_space, v8:isolate:malloc. Petr, Primiano, wdyt?
,
Aug 1 2016
I see two things here:
1. A more general tech problem: how to conveniently extract info in the memory TBM metric.
2. How to deal with these specific V8 values.
As regards 1, I personally like the idea of a selector building block here. We could use it both "internally" (for the "base" memory metric values) and perhaps expose externally (so one can extend the default metric with extra details). Your Opt. 2 there seems pretty simple and powerful at the same time, I like it.
I am not 100% whether we should encourage people to have their external extra values though. I worry about fragmentation here, where some numbers appear only on some benchmarks, and that causes a bit of confusion. On the other side I can imagine how some benchmarks want to measure something really specific (I am thinking to the code size stats for the Ignition benchmark for instance).
Defer this decision to Petr, but on this topic I'd be in favor of saying: try hard to have values plumbed in the base memory metric by default, unless there is a reason not (too spammy, hit some scalability issue, etc).
In essence, I'd like now to have as few special snowflakes as possible, but having been in chrome for a while I feel it is going to be unrealistic to have none.
2. Regarding this specific value. First of all, I'd be in favor of having the heap stats plumbed in the reference memory metric. They can reveal useful information even to non-v8 folks (I imagine somebody doing a change in Oilpan which causes side effects on v8 and they want to get more info out of that).
I wonder if we could know which isolate is the main thread isolate, and could cluster them into something like:
memory...reported_by_chrome:v8:isolates:main_thread:{code_space,old_space,...}
memory...reported_by_chrome:v8:isolates:workers:{code_space,old_space,...}
would it make sense?
we definitely don't want the isolate 0x1234 ptr to make it through the metric, as that would change at every run.
,
Aug 1 2016
Thank you, Primiano.
If we all agree that V8 heap space stats are useful enough to be in the reference memory metric, then I could add them to the memory metric without introducing the generic solution. Similar to what I did with v8/malloc.
I would be happy even if we do not separate main_thread and workers, i.e. combining heap stats over all isolates would already be great:
memory...reported_by_chrome:v8:heap_spaces:{code_space,old_space,...}
I think to distinguish between main_thread and workers we would need to pass a bit in V8 memory dump.
,
Aug 3 2016
(Apologies for taking so long to reply) I agree with Primiano. Until we run into meta-performance issues (the memory metric being too slow or generating data), there's *no* need to *not* report everything all the time.
,
Aug 4 2016
Plan in #3 SGTM.
Maybe we should just be a bit future proof and call the metric today:
memory...reported_by_chrome:v8:***all_isolates***:heap_spaces:{code_space,old_space,...}
(without ***)
So that if in future we add individual isolates the old data still makes sense.
Petr WDYT?
,
Aug 4 2016
#6: I'm a little worried that it's too late for this because we're already reporting 'allocated_by_malloc' without 'all_isolates'. So we'd have to rename old values to make it consistent. Ulan: WDYT?
,
Aug 4 2016
Looks like we cannot avoid renaming if we add individual isolates. Since we don't know whether we will add individual isolates or not, let's postpone renaming? So I would keep v8:heap:...
,
Aug 4 2016
I think I agree. Let's do the renaming in bulk when/if needed.
,
Aug 4 2016
Ah I see, good point. nevermind then.
,
Aug 4 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5c76e52cb7c664bb2d58cccdb6782b4a588e6be4 commit 5c76e52cb7c664bb2d58cccdb6782b4a588e6be4 Author: catapult-deps-roller <catapult-deps-roller@chromium.org> Date: Thu Aug 04 15:30:18 2016 Roll src/third_party/catapult/ c1a1d1b3e..ae72aad91 (2 commits). https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/c1a1d1b3ed6c..ae72aad918e0 $ git log c1a1d1b3e..ae72aad91 --date=short --no-merges --format='%ad %ae %s' BUG= 633160 TBR=catapult-sheriff@chromium.org Review-Url: https://codereview.chromium.org/2211003002 Cr-Commit-Position: refs/heads/master@{#409792} [modify] https://crrev.com/5c76e52cb7c664bb2d58cccdb6782b4a588e6be4/DEPS
,
Aug 30 2016
Fixed by #12. |
||
►
Sign in to add a comment |
||
Comment 1 by sullivan@chromium.org
, Aug 1 2016