Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1908513002:
Add mutator utilization metric. (Closed)
DescriptionAdd mutator utilization metric.
The metric was introduced in "A Parallel, Real-Time Garbage Collector"
paper by Cheng and Blelloch. Since then it became one of the main
metrics for measuring GC performance.
Mutator utilization for a windows size w is a function of time
mu_w(t) that shows how much time in [t, t+w] is left for the mutator.
More formally: mu_w(t) = (w - total_time_spent_in_gc_in(t, t + w)) / w.
This patch computes minimum value and percentiles of mutator utilization
for windows size 16.67, which the frame time corresponding to 60 FPS.
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4e6654209707527b1e91268f7c81d77c4edc8142
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : Move PiecewiseLinearFunction to base, use findTopmostSlicesRelativeToThisSlice #
Total comments: 12
Patch Set 4 : Add more detailed description of mutator utilization #Patch Set 5 : use tr.b.mergeRanges #
Total comments: 88
Patch Set 6 : Address Petr's comments #Patch Set 7 : More fixes #
Total comments: 32
Patch Set 8 : Address Petr's comments #Patch Set 9 : Fix pct naming #
Total comments: 4
Patch Set 10 : Address Ben's comments #
Total comments: 6
Patch Set 11 : Address Petr's comments #Patch Set 12 : Remove redundant semicolon #
Messages
Total messages: 38 (14 generated)
Description was changed from ========== Add mutator utilization metric. BUG=catapult:# ========== to ========== Add mutator utilization metric. ==========
ulan@chromium.org changed reviewers: + eakuefner@chromium.org, hpayer@chromium.org
Ethan, could you please take a look at v8/gc_metric.html ? Hannes, could you please take a look at the rest?
https://codereview.chromium.org/1908513002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/gc_metric.html (right): https://codereview.chromium.org/1908513002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:206: topEvent.descendentSlices.forEach(function(e) { One thing overall about this metric that this CL caused me to notice is that you shouldn't have to use your custom utils to do this type of iteration. You should look into replacing the util functions with model.findTopmostSlicesNamed, or if you need more general predicates than that, model.findTopmostSlices, and refactoring groupAndProcessEvents to take this into account. I think using these helpers throughout will increase the clarity substantially, and we use them in the execution metric to avoid the double-counting problem. Take a look at the docstring to see how it works: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/mode... and at the docstring for the related Slice.findTopmostSlicesRelativeToThisSlice: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/mode... https://codereview.chromium.org/1908513002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:110: You should look into base/ to see if there's math that you can reuse here. https://github.com/catapult-project/catapult/tree/master/tracing/tracing/base math and range_utils may be of particular interest. If there are any math basics that base doesn't provide that you need, you should likely implement them there instead of here.
Thanks! https://codereview.chromium.org/1908513002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/gc_metric.html (right): https://codereview.chromium.org/1908513002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:206: topEvent.descendentSlices.forEach(function(e) { On 2016/04/20 17:25:54, eakuefner wrote: > One thing overall about this metric that this CL caused me to notice is that you > shouldn't have to use your custom utils to do this type of iteration. > > You should look into replacing the util functions with > model.findTopmostSlicesNamed, or if you need more general predicates than that, > model.findTopmostSlices, and refactoring groupAndProcessEvents to take this into > account. I think using these helpers throughout will increase the clarity > substantially, and we use them in the execution metric to avoid the > double-counting problem. > > Take a look at the docstring to see how it works: > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/mode... > > and at the docstring for the related Slice.findTopmostSlicesRelativeToThisSlice: > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/mode... Partially done. Replaced descendentSlices with findTopmostSlicesRelativeToThisSlice The model.findTopmostSlicesNamed it didn't work out: model.findTopmostSlices doesn't know about user expectations, but we are interested in events per user expectation. Also note that we rely on the order of events (line 200) to shift the GC pauses in time. This is cannot be handled in a single event callback of findTopmostSlices. https://codereview.chromium.org/1908513002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:110: On 2016/04/20 17:25:54, eakuefner wrote: > You should look into base/ to see if there's math that you can reuse here. > https://github.com/catapult-project/catapult/tree/master/tracing/tracing/base > > math and range_utils may be of particular interest. If there are any math basics > that base doesn't provide that you need, you should likely implement them there > instead of here. Done. Moved PiecewiseLinearFunction to base and renamed Segment to Piece since it is not a general math segment but representation of PLF piece.
eakuefner@chromium.org changed reviewers: + benjhayden@chromium.org, petrcermak@chromium.org
Adding Ben + Petr because I'm going to be OOO for the next few days, and slow next week. Petr: Can you take a look especially at the math stuff Ulan is adding to base/, as well as v8/util to suggest any alternatives to the additional math stuff there? Ben: EventSet isn't an EventContainer so findTopmostSlices is unavailable for ue.associatedEvents, which means that Ulan has to do a lot of iteration by hand. I'm wondering 1. whether you have any tips for structuring this per-UE metric and 2. whether it would be feasible to make EventSet an EventContainer so we can use findTopmostSlices.
Would you mind expanding the CL description to include a few things? Briefly, what is "mutator utilization"? Why do we need it? Is there a bug that you can link to that contains more background and context? For the CL, I might be wrong, but it looks like there might be some wheel-re-invention. If you can't use one of the existing alternatives, can you explain why not? Thanks! https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/base/pi... File tracing/tracing/base/piecewise_linear_function.html (right): https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:19: function PiecewiseLinearFunction() { This looks a lot like Numeric.getInterpolatedCountAt() and vec2.interpolatePiecewiseFunction. Can you use either of those instead? Or refactor things to share more code? https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:92: this.x1 = x1; Can you use vec2? https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:112: * Given a list of intervals, returns a new list with all overalapping Can you use tr.b.mergeRanges() from range_utils.html instead? https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:138: * See "A Parallel, Real-Time Garbage Collector" by Cheng et. al. for Ok, but for those of us who don't have time to read 11 pages of technical writing in 6pt font, plus references, can you summarize the relevant parts of the definition? And maybe also link directly to it so we can be sure we find the correct version? https://www.cs.cmu.edu/~guyb/papers/gc2001.pdf
Sorry, I haven't even gotten to Ethan's question yet. I'm starting to think that we could use some sort of documentation to help people find tools that already exist. Thoughts? https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... File tracing/tracing/metrics/v8/gc_metric.html (right): https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:245: function groupAndProcessEvents(model, filterCallback, Can you use EventSet.getEventsOrganizedByCallback() instead? https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:250: stageTitleToNameToEvents[ue.stageTitle] = Sorry, the "title" part of the field name is supposed to indicate that it's for display purposes only. We might change it at any time with no notice for trivial aesthetic reasons. Can you use instanceof Load/Animation/Idle/ResponseExpectation instead?
Ok, to answer Ethan's question, it looks like if you were to add EventSet.findTopmostSlices, it would basically do what ulan is doing here, is that right? For each event in the EventSet, if event instanceof Slice, event.findTopmostSlicesRelativeToThisSlice(). Sure, you could do that, and I can see how it might be nice to have a little more parity between EventSet and EventContainer, but it seems like it would only buy you a line or two here. Do you expect more callers? Or do you want to explore unifying EventSet and EventContainer totally?
Description was changed from ========== Add mutator utilization metric. ========== to ========== Add mutator utilization metric. The metric was introduced in "A Parallel, Real-Time Garbage Collector" paper by Cheng and Blelloch. Since then it became one of the main metrics for measuring GC performance. Mutator utilization for a windows size w is a function of time mu_w(t) that shows how much time in [t, t+w] is left for the mutator. More formally: mu_w(t) = (W - total_time_spent_in_gc_in(t, t + W)) / W. This patch computes minimum value and percentiles for mu_16(t). ==========
Description was changed from ========== Add mutator utilization metric. The metric was introduced in "A Parallel, Real-Time Garbage Collector" paper by Cheng and Blelloch. Since then it became one of the main metrics for measuring GC performance. Mutator utilization for a windows size w is a function of time mu_w(t) that shows how much time in [t, t+w] is left for the mutator. More formally: mu_w(t) = (W - total_time_spent_in_gc_in(t, t + W)) / W. This patch computes minimum value and percentiles for mu_16(t). ========== to ========== Add mutator utilization metric. The metric was introduced in "A Parallel, Real-Time Garbage Collector" paper by Cheng and Blelloch. Since then it became one of the main metrics for measuring GC performance. Mutator utilization for a windows size w is a function of time mu_w(t) that shows how much time in [t, t+w] is left for the mutator. More formally: mu_w(t) = (w - total_time_spent_in_gc_in(t, t + w)) / w. This patch computes minimum value and percentiles for mu_16(t). ==========
Thanks a lot, Ben. I updated the description and uploaded new patch set. https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/base/pi... File tracing/tracing/base/piecewise_linear_function.html (right): https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:19: function PiecewiseLinearFunction() { On 2016/04/20 23:03:18, benjhayden_chromium wrote: > This looks a lot like Numeric.getInterpolatedCountAt() and > vec2.interpolatePiecewiseFunction. Can you use either of those instead? Or > refactor things to share more code? Nope, this code it uses mathematical properties of linearly interpolated function to compute min, max, average, percentile of the function. It doesn't actually interpolate. https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:92: this.x1 = x1; On 2016/04/20 23:03:18, benjhayden_chromium wrote: > Can you use vec2? Yes, vec2 can be used to store the points, but I don't think it would improve the code. Instead of this.x1 we would have this.p1[0], which is more verbose and less readable. And there are not function in vec2 that I can use here. https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... File tracing/tracing/metrics/v8/gc_metric.html (right): https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:245: function groupAndProcessEvents(model, filterCallback, On 2016/04/20 23:11:54, benjhayden_chromium wrote: > Can you use EventSet.getEventsOrganizedByCallback() instead? Unfortunately no. The problem is that model.userModel doesn't give grouped user expectations. For example, if we have model.userModel.expectation = [Animation, Idle, Animation] then we would process the two Animation user expecations separately. In our metrics we want to group user expectations. That's was groupAndProcessEvents achieves. Would the grouping function in UserModel be useful for other metrics? If yes, I could add it there. I am also happy with the current state. https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:250: stageTitleToNameToEvents[ue.stageTitle] = On 2016/04/20 23:11:54, benjhayden_chromium wrote: > Sorry, the "title" part of the field name is supposed to indicate that it's for > display purposes only. We might change it at any time with no notice for trivial > aesthetic reasons. > > Can you use instanceof Load/Animation/Idle/ResponseExpectation instead? The change is stageTitle would be fine as long as it is different for different stages. That is the only property we use here. And we actually want display friendly name because we using the title as part of value names. By the way, wouldn't "instanceof X" break the abstraction of user expectation? https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:112: * Given a list of intervals, returns a new list with all overalapping On 2016/04/20 23:03:18, benjhayden_chromium wrote: > Can you use tr.b.mergeRanges() from range_utils.html instead? Done. Thanks! https://codereview.chromium.org/1908513002/diff/10004/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:138: * See "A Parallel, Real-Time Garbage Collector" by Cheng et. al. for On 2016/04/20 23:03:18, benjhayden_chromium wrote: > Ok, but for those of us who don't have time to read 11 pages of technical > writing in 6pt font, plus references, can you summarize the relevant parts of > the definition? > And maybe also link directly to it so we can be sure we find the correct > version? https://www.cs.cmu.edu/~guyb/papers/gc2001.pdf Done, but I don't expect reviewer from Telemetry to spend time understanding this, because it is GC specific. I added Hanner from to review correctness of this function.
My first pass of comments. Please break lines in the patch description at 72 characters (except for URLs and the first line containing the patch title). Thanks, Petr https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... File tracing/tracing/base/piecewise_linear_function.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:15: * A function that consists of linear pieces. The "*" symbols should be aligned with the first "*" symbol on line 14: /** * A function ... * more text ... */ Please fix this everywhere https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:25: * Push a linear piece defined by linear interpolation of supernit: s/of/between/? https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:29: if (x1 > x2) What about x1 === x2? If this is ignored on purpose, please add a comment saying it. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:36: * The total length of all x points for which f(x) < y. What does "length of points" mean??? https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:39: return this.pieces.reduce((acc, x) => (acc + x.partBelow(y)), 0); s/x/p/ because 'x' suggest that it's an x-coordinate. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:57: if (totalWeight == 0) nit: === https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:64: * that have f(x) <= y is equal to the given |percent|. nit: s/equal/approximately equal/ https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:72: var PRECISION = 1e-7; this should be a top-level constant https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:73: if (total == 0) return 0; ===, please put the return statement on a separate line https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:106: if (width == 0) return 0; ===, please put the return statement on a separate line https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... File tracing/tracing/base/piecewise_linear_function_test.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function_test.html:7: <link rel="import" href="/tracing/base/piecewise_linear_function.html"> nit: add blank line above this line https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function_test.html:13: test('PiecewiseLinearFunction', function() { Could you test some edge cases as well (e.g. zero pieces) https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function_test.html:18: assert.equal(1.0, f.max); s/equal/strictEqual/g https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function_test.html:18: assert.equal(1.0, f.max); in catapult, we use the convention assert.someTestFunction(actualValue, expectedValue) https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/gc_metric.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:196: tr.metrics.v8.utils.isTopV8ExecuteEvent, nit: lines 196-231 should be indented 4 spaces (+2) https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:209: pauses.push({start: start, end: end}); I'd say there's no point in naming the local variables. Why not just construct the dictionary straightaway: pauses.push({ start: e.start - topEvent.start + time, end: ... }); https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:217: tr.metrics.v8.utils.mutatorUtilization(0, time, 16, pauses); Where does the 16 come from? Please turn this into a named constant. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:222: new tr.v.NumericValue(model.canonicalUrl, nit: this should be indented 4 spaces (ditto next lines) https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:229: new tr.v.NumericValue(model.canonicalUrl, ditto indentation https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/gc_metric_test.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric_test.html:233: 0, 300, 16, [{start: 50, end: 60}, {start: 110, end: 120}]); Use a constant instead of the mysterious 16 https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric_test.html:234: var pct95 = mutatorUtilization.percentile(1 - 0.95) * 100; I think you're doing way too much work on lines 234-244. The following would perfectly suffice: assert.strictEqual( actual['Animation-mutator_utilization_pct95'], mutatorUtilization.percentile(1 - 0.95) * 100); assert.strictEqual( actual['Animation-mutator_utilization_pct99'], mutatorUtilization.percentile(1 - 0.99) * 100); https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:117: if (intervals.length == 0) === https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:118: return intervals; I'd suggest returning [] so that no matter what you pass into this method, modifying the result won't affect |intervals|. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:132: * Mutator utilization for a windows size w is a function of time mu_w(t) s/windows/window/ https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:138: * @param {Number} start The start time of execution. nit: add a blank line ave the first param description. s/Number/number/ (also applies to lines 139 and 140) https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:141: * @param {Array} intervals The list of GC pauses, where each interval nit: !Array<!{start: number, end: number}> Note that if you do this, there won't be any need for ", where each interval ..." ;-) https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:146: if (end - start <= timeWindow) Please add some comments explaining these cases, e.g.: // Case 1: bla bla bla if (end - start <= timeWindow) return mu // Case 2: foo foo foo if (intervals.length === 0) ... https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:148: if (intervals.length == 0) { === https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:152: intervals = unionOfIntervals(intervals); I'm sure you know what you are doing here, but please add some comments to the code explaining what you're doing. Also consider adding blank lines to split the large function into smaller blocks. If you can, factor out more code into more helper functions. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:174: while (right.position - left.position < timeWindow) { nit: no need for braces https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:188: function advance(point, delta) { Define this in the outer scope. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/utils_test.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:8: <link rel="import" href="/tracing/core/test_utils.html"> no need for this https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:21: * Brute force computation of the mutator utilization. ditto "*" alignment https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:27: function timeToIndex(time) { define this in the (test) outer scope https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:32: var bitmap = new Array(N + 1); it seems to me that bitmap[N] and bitmap[N - 1] will always be |false| since bitmap.length = timeToIndex(end) + 2. Is that intended? https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:33: for (var i = 0; i <= N; i++) { nit: no need for braces. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:39: for (var i = start; i < end; i++) { nit: no need for braces. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:43: var pause = new Array(N); don't you mean N + 1? https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:48: var mu = new Array(); mu = new Array(Math.max(N + 1 - windowWidth, 0)); for (var i = 0; i < mu.length; i++) { var value = pause[i + windowWidth] + pause[i]; mu[i] = 1.0 - value / windowWidth; } https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:54: var max = mu.reduce((acc, x) => Math.max(acc, x), 0); Why do you define this when you later re-calculate it anyway (on line 63)? Alternatively, why don't you calculate the four values upfront? https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:72: var list, union; I don't think there any value in defining |list| and |union|. I'd just do the following: assert.deepEqual( tr.metrics.v8.utils.unionOfIntervals([...]), [...]); Note that we do (actual, expected) as mentioned earlier. Also, if you want to write a little less code, feel free to do the following on line 15: var unionOfIntervals = tr.metrics.v8.utils.unionOfIntervals; Lots of tests do this ;-) https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:97: var pauses, actual, expected; I don't think you need |pauses| and |actual| to be local variables. Furthermore, consider defining a PiecewiseLinearFunction.fromPieces(...) static method so that you could inline everything into a single call. If you don't want to expose such a method, then I suggest you define it in the test, e.g.: function createExpectedFunction(pieces) { var f = new PiecewiseLinearFunction(); pieces.forEach(function(p) { f.push(p.x1, p.y1, p.x2, p.y2); }); return f; } and then do: assert.deepEqual( tr.metrics.v8.utils.mutatorUtilization(...), createExpectedFunction([...])); If you really really really want to do it this way, please at least add blank lines between the sub-cases for better readability (applies to the other tests as well). https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:123: assert.equal(JSON.stringify(expected), JSON.stringify(actual)); I don't think you need this
Also please explain why you chose to expose mu_16(t) (and not mu_32(x) etc.). Thanks, Petr https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... File tracing/tracing/base/piecewise_linear_function.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:19: function PiecewiseLinearFunction() { There's nothing preventing you from adding two pieces that overlap and there might be gaps between the pieces, so I this is not really a piecewise linear function in my opinion. It's really just a "list of linear pieces".
lgtm
Description was changed from ========== Add mutator utilization metric. The metric was introduced in "A Parallel, Real-Time Garbage Collector" paper by Cheng and Blelloch. Since then it became one of the main metrics for measuring GC performance. Mutator utilization for a windows size w is a function of time mu_w(t) that shows how much time in [t, t+w] is left for the mutator. More formally: mu_w(t) = (w - total_time_spent_in_gc_in(t, t + w)) / w. This patch computes minimum value and percentiles for mu_16(t). ========== to ========== Add mutator utilization metric. The metric was introduced in "A Parallel, Real-Time Garbage Collector" paper by Cheng and Blelloch. Since then it became one of the main metrics for measuring GC performance. Mutator utilization for a windows size w is a function of time mu_w(t) that shows how much time in [t, t+w] is left for the mutator. More formally: mu_w(t) = (w - total_time_spent_in_gc_in(t, t + w)) / w. This patch computes minimum value and percentiles of mutator utilization for windows size 16.67, which the frame time corresponding to 60 FPS. ==========
Description was changed from ========== Add mutator utilization metric. The metric was introduced in "A Parallel, Real-Time Garbage Collector" paper by Cheng and Blelloch. Since then it became one of the main metrics for measuring GC performance. Mutator utilization for a windows size w is a function of time mu_w(t) that shows how much time in [t, t+w] is left for the mutator. More formally: mu_w(t) = (w - total_time_spent_in_gc_in(t, t + w)) / w. This patch computes minimum value and percentiles of mutator utilization for windows size 16.67, which the frame time corresponding to 60 FPS. ========== to ========== Add mutator utilization metric. The metric was introduced in "A Parallel, Real-Time Garbage Collector" paper by Cheng and Blelloch. Since then it became one of the main metrics for measuring GC performance. Mutator utilization for a windows size w is a function of time mu_w(t) that shows how much time in [t, t+w] is left for the mutator. More formally: mu_w(t) = (w - total_time_spent_in_gc_in(t, t + w)) / w. This patch computes minimum value and percentiles of mutator utilization for windows size 16.67, which the frame time corresponding to 60 FPS. ==========
Thanks a lot! https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... File tracing/tracing/base/piecewise_linear_function.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:15: * A function that consists of linear pieces. On 2016/04/21 13:42:29, petrcermak wrote: > The "*" symbols should be aligned with the first "*" symbol on line 14: > > /** > * A function ... > * more text ... > */ > > Please fix this everywhere Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:19: function PiecewiseLinearFunction() { On 2016/04/21 13:45:22, petrcermak wrote: > There's nothing preventing you from adding two pieces that overlap and there > might be gaps between the pieces, so I this is not really a piecewise linear > function in my opinion. It's really just a "list of linear pieces". Good point. Added a (conservative) check for overlapping pieces. The gaps should be OK, the function will be not continious in that case. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:25: * Push a linear piece defined by linear interpolation of On 2016/04/21 13:42:29, petrcermak wrote: > supernit: s/of/between/? Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:29: if (x1 > x2) On 2016/04/21 13:42:29, petrcermak wrote: > What about x1 === x2? If this is ignored on purpose, please add a comment saying > it. Done. Disallowing x1 === x2. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:36: * The total length of all x points for which f(x) < y. On 2016/04/21 13:42:30, petrcermak wrote: > What does "length of points" mean??? Done. Rephrased. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:39: return this.pieces.reduce((acc, x) => (acc + x.partBelow(y)), 0); On 2016/04/21 13:42:29, petrcermak wrote: > s/x/p/ because 'x' suggest that it's an x-coordinate. Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:57: if (totalWeight == 0) On 2016/04/21 13:42:29, petrcermak wrote: > nit: === Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:64: * that have f(x) <= y is equal to the given |percent|. On 2016/04/21 13:42:29, petrcermak wrote: > nit: s/equal/approximately equal/ Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:72: var PRECISION = 1e-7; On 2016/04/21 13:42:29, petrcermak wrote: > this should be a top-level constant Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:73: if (total == 0) return 0; On 2016/04/21 13:42:30, petrcermak wrote: > ===, please put the return statement on a separate line Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function.html:106: if (width == 0) return 0; On 2016/04/21 13:42:29, petrcermak wrote: > ===, please put the return statement on a separate line Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... File tracing/tracing/base/piecewise_linear_function_test.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function_test.html:7: <link rel="import" href="/tracing/base/piecewise_linear_function.html"> On 2016/04/21 13:42:30, petrcermak wrote: > nit: add blank line above this line Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function_test.html:13: test('PiecewiseLinearFunction', function() { On 2016/04/21 13:42:30, petrcermak wrote: > Could you test some edge cases as well (e.g. zero pieces) Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function_test.html:18: assert.equal(1.0, f.max); On 2016/04/21 13:42:30, petrcermak wrote: > s/equal/strictEqual/g Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/base/pi... tracing/tracing/base/piecewise_linear_function_test.html:18: assert.equal(1.0, f.max); On 2016/04/21 13:42:30, petrcermak wrote: > in catapult, we use the convention assert.someTestFunction(actualValue, > expectedValue) Done. Good to know! https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/gc_metric.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:196: tr.metrics.v8.utils.isTopV8ExecuteEvent, On 2016/04/21 13:42:30, petrcermak wrote: > nit: lines 196-231 should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:209: pauses.push({start: start, end: end}); On 2016/04/21 13:42:30, petrcermak wrote: > I'd say there's no point in naming the local variables. Why not just construct > the dictionary straightaway: > > pauses.push({ > start: e.start - topEvent.start + time, > end: ... > }); Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:217: tr.metrics.v8.utils.mutatorUtilization(0, time, 16, pauses); On 2016/04/21 13:42:30, petrcermak wrote: > Where does the 16 come from? Please turn this into a named constant. Done. Added comment. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:222: new tr.v.NumericValue(model.canonicalUrl, On 2016/04/21 13:42:30, petrcermak wrote: > nit: this should be indented 4 spaces (ditto next lines) Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric.html:229: new tr.v.NumericValue(model.canonicalUrl, On 2016/04/21 13:42:30, petrcermak wrote: > ditto indentation Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/gc_metric_test.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric_test.html:233: 0, 300, 16, [{start: 50, end: 60}, {start: 110, end: 120}]); On 2016/04/21 13:42:30, petrcermak wrote: > Use a constant instead of the mysterious 16 Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/gc_metric_test.html:234: var pct95 = mutatorUtilization.percentile(1 - 0.95) * 100; On 2016/04/21 13:42:30, petrcermak wrote: > I think you're doing way too much work on lines 234-244. The following would > perfectly suffice: > > assert.strictEqual( > actual['Animation-mutator_utilization_pct95'], > mutatorUtilization.percentile(1 - 0.95) * 100); > assert.strictEqual( > actual['Animation-mutator_utilization_pct99'], > mutatorUtilization.percentile(1 - 0.99) * 100); Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:117: if (intervals.length == 0) On 2016/04/21 13:42:30, petrcermak wrote: > === Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:118: return intervals; On 2016/04/21 13:42:30, petrcermak wrote: > I'd suggest returning [] so that no matter what you pass into this method, > modifying the result won't affect |intervals|. Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:132: * Mutator utilization for a windows size w is a function of time mu_w(t) On 2016/04/21 13:42:30, petrcermak wrote: > s/windows/window/ Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:138: * @param {Number} start The start time of execution. On 2016/04/21 13:42:30, petrcermak wrote: > nit: add a blank line ave the first param description. > > s/Number/number/ (also applies to lines 139 and 140) Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:141: * @param {Array} intervals The list of GC pauses, where each interval On 2016/04/21 13:42:30, petrcermak wrote: > nit: !Array<!{start: number, end: number}> > > Note that if you do this, there won't be any need for ", where each interval > ..." ;-) Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:146: if (end - start <= timeWindow) On 2016/04/21 13:42:30, petrcermak wrote: > Please add some comments explaining these cases, e.g.: > > > // Case 1: bla bla bla > if (end - start <= timeWindow) > return mu > > // Case 2: foo foo foo > if (intervals.length === 0) > ... Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:148: if (intervals.length == 0) { On 2016/04/21 13:42:30, petrcermak wrote: > === Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:152: intervals = unionOfIntervals(intervals); On 2016/04/21 13:42:30, petrcermak wrote: > I'm sure you know what you are doing here, but please add some comments to the > code explaining what you're doing. Also consider adding blank lines to split the > large function into smaller blocks. If you can, factor out more code into more > helper functions. Done. Extracted the left and right endpoints of the windows into a separate class. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:174: while (right.position - left.position < timeWindow) { On 2016/04/21 13:42:30, petrcermak wrote: > nit: no need for braces Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils.html:188: function advance(point, delta) { On 2016/04/21 13:42:30, petrcermak wrote: > Define this in the outer scope. Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... File tracing/tracing/metrics/v8/utils_test.html (right): https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:8: <link rel="import" href="/tracing/core/test_utils.html"> On 2016/04/21 13:42:31, petrcermak wrote: > no need for this Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:21: * Brute force computation of the mutator utilization. On 2016/04/21 13:42:31, petrcermak wrote: > ditto "*" alignment Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:27: function timeToIndex(time) { On 2016/04/21 13:42:31, petrcermak wrote: > define this in the (test) outer scope It depends on the STEP. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:32: var bitmap = new Array(N + 1); On 2016/04/21 13:42:31, petrcermak wrote: > it seems to me that bitmap[N] and bitmap[N - 1] will always be |false| since > bitmap.length = timeToIndex(end) + 2. Is that intended? You're right N + 1 is not needed. Fixed. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:33: for (var i = 0; i <= N; i++) { On 2016/04/21 13:42:31, petrcermak wrote: > nit: no need for braces. Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:39: for (var i = start; i < end; i++) { On 2016/04/21 13:42:31, petrcermak wrote: > nit: no need for braces. Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:43: var pause = new Array(N); On 2016/04/21 13:42:31, petrcermak wrote: > don't you mean N + 1? I fixed the loop limit below. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:48: var mu = new Array(); On 2016/04/21 13:42:31, petrcermak wrote: > mu = new Array(Math.max(N + 1 - windowWidth, 0)); > for (var i = 0; i < mu.length; i++) { > var value = pause[i + windowWidth] + pause[i]; > mu[i] = 1.0 - value / windowWidth; > } Done. Thanks. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:54: var max = mu.reduce((acc, x) => Math.max(acc, x), 0); On 2016/04/21 13:42:31, petrcermak wrote: > Why do you define this when you later re-calculate it anyway (on line 63)? > Alternatively, why don't you calculate the four values upfront? Yep, thanks. That's what I wanted to do but forgot. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:72: var list, union; On 2016/04/21 13:42:31, petrcermak wrote: > I don't think there any value in defining |list| and |union|. I'd just do the > following: > > assert.deepEqual( > tr.metrics.v8.utils.unionOfIntervals([...]), > [...]); > > Note that we do (actual, expected) as mentioned earlier. Also, if you want to > write a little less code, feel free to do the following on line 15: > > var unionOfIntervals = tr.metrics.v8.utils.unionOfIntervals; > > Lots of tests do this ;-) Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:97: var pauses, actual, expected; On 2016/04/21 13:42:31, petrcermak wrote: > I don't think you need |pauses| and |actual| to be local variables. > > Furthermore, consider defining a PiecewiseLinearFunction.fromPieces(...) static > method so that you could inline everything into a single call. If you don't want > to expose such a method, then I suggest you define it in the test, e.g.: > > function createExpectedFunction(pieces) { > var f = new PiecewiseLinearFunction(); > pieces.forEach(function(p) { > f.push(p.x1, p.y1, p.x2, p.y2); > }); > return f; > } > > and then do: > > assert.deepEqual( > tr.metrics.v8.utils.mutatorUtilization(...), > createExpectedFunction([...])); > > If you really really really want to do it this way, please at least add blank > lines between the sub-cases for better readability (applies to the other tests > as well). Done. https://codereview.chromium.org/1908513002/diff/70001/tracing/tracing/metrics... tracing/tracing/metrics/v8/utils_test.html:123: assert.equal(JSON.stringify(expected), JSON.stringify(actual)); On 2016/04/21 13:42:31, petrcermak wrote: > I don't think you need this Thanks. That's debugging leftover. Removed.
I think this is almost ready to land. Thanks for all the helpful comments :-) Petr https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/base/p... File tracing/tracing/base/piecewise_linear_function.html (right): https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/base/p... tracing/tracing/base/piecewise_linear_function.html:28: * Pieces should be pushed in the order of increasing x coordinate. nit: s/should/must/ https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/base/p... File tracing/tracing/base/piecewise_linear_function_test.html (right): https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/base/p... tracing/tracing/base/piecewise_linear_function_test.html:20: }); nit: add a blank line after this one https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... File tracing/tracing/metrics/v8/gc_metric.html (right): https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/gc_metric.html:197: var WINDOW_SIZE = 16.67; please move this to the module scope (after line 17). https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/gc_metric.html:219: tr.metrics.v8.utils.mutatorUtilization( nit: would this really not fit on the previous line? https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/gc_metric.html:232: new tr.v.NumericValue(model.canonicalUrl, ditto https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... File tracing/tracing/metrics/v8/gc_metric_test.html (right): https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/gc_metric_test.html:213: var WINDOW_SIZE = 16.67; Instead of defining the same constant twice, I suggest you export it from the gc_metric: return { gcMetric: gcMetric, WINDOW_SIZE: WINDOW_SIZE // For testing purposes only. }; https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:132: * over the |points| in the time interval defined by the |start| and |end|. nit: s/the |points|/|points|/ and s/the |start|/|start|/ https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:136: function WindowEndpoint(start, end, points) { Why do you pass in |end| when you don't use it??? https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:139: this.index = -1; lastIndex? https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:140: // The position of the limit. limit of what? https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:141: this.position = start; limitPosition? https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:143: this.remainder = points[0].position - start; distanceUntilNextPoint? https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:145: this.pause = 0; cumulativePause? https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:147: this.stack = 0; stackDepth? https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:162: this.index += 1; this.index++ https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:189: // If the interval is smaller than the time windows, then the function is nit: s/windows/window/
Good suggestions! https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/base/p... File tracing/tracing/base/piecewise_linear_function.html (right): https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/base/p... tracing/tracing/base/piecewise_linear_function.html:28: * Pieces should be pushed in the order of increasing x coordinate. On 2016/04/22 11:40:01, petrcermak wrote: > nit: s/should/must/ Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/base/p... File tracing/tracing/base/piecewise_linear_function_test.html (right): https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/base/p... tracing/tracing/base/piecewise_linear_function_test.html:20: }); On 2016/04/22 11:40:01, petrcermak wrote: > nit: add a blank line after this one Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... File tracing/tracing/metrics/v8/gc_metric.html (right): https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/gc_metric.html:197: var WINDOW_SIZE = 16.67; On 2016/04/22 11:40:01, petrcermak wrote: > please move this to the module scope (after line 17). Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/gc_metric.html:219: tr.metrics.v8.utils.mutatorUtilization( On 2016/04/22 11:40:01, petrcermak wrote: > nit: would this really not fit on the previous line? Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/gc_metric.html:232: new tr.v.NumericValue(model.canonicalUrl, On 2016/04/22 11:40:01, petrcermak wrote: > ditto Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... File tracing/tracing/metrics/v8/gc_metric_test.html (right): https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/gc_metric_test.html:213: var WINDOW_SIZE = 16.67; On 2016/04/22 11:40:01, petrcermak wrote: > Instead of defining the same constant twice, I suggest you export it from the > gc_metric: > > return { > gcMetric: gcMetric, > WINDOW_SIZE: WINDOW_SIZE // For testing purposes only. > }; Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:132: * over the |points| in the time interval defined by the |start| and |end|. On 2016/04/22 11:40:02, petrcermak wrote: > nit: s/the |points|/|points|/ and s/the |start|/|start|/ Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:136: function WindowEndpoint(start, end, points) { On 2016/04/22 11:40:01, petrcermak wrote: > Why do you pass in |end| when you don't use it??? Leftover, removed. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:139: this.index = -1; On 2016/04/22 11:40:01, petrcermak wrote: > lastIndex? Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:140: // The position of the limit. On 2016/04/22 11:40:02, petrcermak wrote: > limit of what? Fixed the comment. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:141: this.position = start; On 2016/04/22 11:40:02, petrcermak wrote: > limitPosition? I removed limit from the comment. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:143: this.remainder = points[0].position - start; On 2016/04/22 11:40:02, petrcermak wrote: > distanceUntilNextPoint? Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:145: this.pause = 0; On 2016/04/22 11:40:02, petrcermak wrote: > cumulativePause? Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:147: this.stack = 0; On 2016/04/22 11:40:02, petrcermak wrote: > stackDepth? Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:162: this.index += 1; On 2016/04/22 11:40:02, petrcermak wrote: > this.index++ Done. https://codereview.chromium.org/1908513002/diff/110001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:189: // If the interval is smaller than the time windows, then the function is On 2016/04/22 11:40:02, petrcermak wrote: > nit: s/windows/window/ Done.
It'd be nice to clarify units. lgtm. https://codereview.chromium.org/1908513002/diff/150001/tracing/tracing/metric... File tracing/tracing/metrics/v8/gc_metric.html (right): https://codereview.chromium.org/1908513002/diff/150001/tracing/tracing/metric... tracing/tracing/metrics/v8/gc_metric.html:20: var WINDOW_SIZE = 16.67; Can you parameterize what you mean, and specify units? var TARGET_FPS = 60; var WINDOW_MS = MS_PER_S / TARGET_FPS; https://codereview.chromium.org/1908513002/diff/150001/tracing/tracing/metric... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/150001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:180: * mu_w(t) = (w - total_time_spent_in_gc_in(t, t + w)) / w. What are the range, domain, and units of t and mu_w()? I assume t, start, end, timeWindow, and intervals are all in ms? Can you add a suffix 'Ms' to the names?
Thanks, Ben. Petr, any more comments? https://codereview.chromium.org/1908513002/diff/150001/tracing/tracing/metric... File tracing/tracing/metrics/v8/gc_metric.html (right): https://codereview.chromium.org/1908513002/diff/150001/tracing/tracing/metric... tracing/tracing/metrics/v8/gc_metric.html:20: var WINDOW_SIZE = 16.67; On 2016/04/22 16:49:39, benjhayden_chromium wrote: > Can you parameterize what you mean, and specify units? > var TARGET_FPS = 60; > var WINDOW_MS = MS_PER_S / TARGET_FPS; Done. https://codereview.chromium.org/1908513002/diff/150001/tracing/tracing/metric... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/150001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:180: * mu_w(t) = (w - total_time_spent_in_gc_in(t, t + w)) / w. On 2016/04/22 16:49:39, benjhayden_chromium wrote: > What are the range, domain, and units of t and mu_w()? > I assume t, start, end, timeWindow, and intervals are all in ms? Can you add a > suffix 'Ms' to the names? Added comments. The function is agnostic of argument units. It works for ms, seconds, hours as long as all arguments have the same time unit.
LGTM with two final comments. Thanks for your patience :-) Petr https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/base/p... File tracing/tracing/base/piecewise_linear_function.html (right): https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/base/p... tracing/tracing/base/piecewise_linear_function.html:83: if (below / total < percent) { nit: no need for braces: if (below / total < percent) lower = middle else upper = middle https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/metric... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:148: }; nit: no need for semicolon here. https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/metric... File tracing/tracing/metrics/v8/utils_test.html (right): https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils_test.html:56: get average() { You're making your code less efficient by wrapping already computed values in getters. Do the following instead: return { average: average_, min: min_, max: max_, percentile: function(percent) { ... } }; Furthermore, there's no need to store the stats in local variables. You can simply calculate them in place: return { average: mu.reduce((acc, x) => Math.max(acc, x), 0), ... };
Thanks, landing. https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/base/p... File tracing/tracing/base/piecewise_linear_function.html (right): https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/base/p... tracing/tracing/base/piecewise_linear_function.html:83: if (below / total < percent) { On 2016/04/25 12:31:17, petrcermak wrote: > nit: no need for braces: > > if (below / total < percent) > lower = middle > else > upper = middle Done. https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/metric... File tracing/tracing/metrics/v8/utils.html (right): https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils.html:148: }; On 2016/04/25 12:31:17, petrcermak wrote: > nit: no need for semicolon here. Done. https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/metric... File tracing/tracing/metrics/v8/utils_test.html (right): https://codereview.chromium.org/1908513002/diff/170001/tracing/tracing/metric... tracing/tracing/metrics/v8/utils_test.html:56: get average() { On 2016/04/25 12:31:17, petrcermak wrote: > You're making your code less efficient by wrapping already computed values in > getters. Do the following instead: > > return { > average: average_, > min: min_, > max: max_, > percentile: function(percent) { > ... > } > }; > > Furthermore, there's no need to store the stats in local variables. You can > simply calculate them in place: > > return { > average: mu.reduce((acc, x) => Math.max(acc, x), 0), > ... > }; Yeah, this code was strange. Changed it as you suggested. Thanks!
The CQ bit was checked by ulan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org, benjhayden@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1908513002/#ps200001 (title: "Remove redundant semicolon")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908513002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908513002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ulan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908513002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908513002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)
The CQ bit was checked by ulan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908513002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908513002/200001
Message was sent while issue was closed.
Description was changed from ========== Add mutator utilization metric. The metric was introduced in "A Parallel, Real-Time Garbage Collector" paper by Cheng and Blelloch. Since then it became one of the main metrics for measuring GC performance. Mutator utilization for a windows size w is a function of time mu_w(t) that shows how much time in [t, t+w] is left for the mutator. More formally: mu_w(t) = (w - total_time_spent_in_gc_in(t, t + w)) / w. This patch computes minimum value and percentiles of mutator utilization for windows size 16.67, which the frame time corresponding to 60 FPS. ========== to ========== Add mutator utilization metric. The metric was introduced in "A Parallel, Real-Time Garbage Collector" paper by Cheng and Blelloch. Since then it became one of the main metrics for measuring GC performance. Mutator utilization for a windows size w is a function of time mu_w(t) that shows how much time in [t, t+w] is left for the mutator. More formally: mu_w(t) = (w - total_time_spent_in_gc_in(t, t + w)) / w. This patch computes minimum value and percentiles of mutator utilization for windows size 16.67, which the frame time corresponding to 60 FPS. Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:200001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
