Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
130613002:
Update unit info file for perf dashboard. (Closed)
|
Created:
6 years, 11 months ago by qyearsley Modified:
6 years, 11 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, victorhsieh Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUpdate unit info file for perf dashboard.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244390
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 13
Patch Set 4 : Updating two of the "why" comments. #Patch Set 5 : "percentage" -> "coverage%" #Messages
Total messages: 13 (0 generated)
Just noticed some units without improvement direction data on the dashboard under ChromiumPerf/cros-alex/dromaeo.domcorequery/dom_query (runs_per_s) and I wanted to quickly check if any other units needed to be update that I could update at the same time. Another unit I noticed doesn't have information is "percentage", used in alloy-perf-test/cts%; victorhsieh, do you happen to know what this metric represents and whether is better or worse?
lgtm But in general, I'd like to either see this file shrink to a minimal set of canonical units or else eliminate this file and have all results carry with them an improvement direction specified by the benchmark. https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.json File tools/perf/unit-info.json (right): https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:82: "percentage": { Wondering if alloy should use a canonical unit instead of adding. https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:98: "why": "Used in dromaeo. Higher runs/ms implies faster execution." Not sure this one is used in dromaeo. Is it? https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:104: "runs_per_s": { Any chance of getting the cros bots to use a canonical unit or should we just bite the bullet and add this?
alloy-perf-test is a performance test category for an internal experimental project, so I'd not discover the detail. If we need to change the unit, please let me know how should we deal with the transition. https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.json File tools/perf/unit-info.json (right): https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:82: "percentage": { On 2014/01/09 03:47:43, tonyg wrote: > Wondering if alloy should use a canonical unit instead of adding. No objection to use canonical unit, but in our case, it's a "up" (the higher the better). Please let me know how should we handle the transition from our code, if the decision is to change this. Thanks.
Another random comment: Another type of units that's being sent that doesn't have an improvement direction is "iterations", as a measure of total test time length, sent by endure. This legitimately doesn't have an "improvement direction", it's just for information purposes, so that's why I'm not adding that. https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.json File tools/perf/unit-info.json (right): https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:82: "percentage": { On 2014/01/09 04:42:11, victorhsieh wrote: > On 2014/01/09 03:47:43, tonyg wrote: > > Wondering if alloy should use a canonical unit instead of adding. > > No objection to use canonical unit, but in our case, it's a "up" (the higher the > better). > > Please let me know how should we handle the transition from our code, if the > decision is to change this. Thanks. Alright, since the improvement direction is different I think it's fine to call it "percentage" and have a comment that higher is better. Or, Victor, if you think it makes more sense and you want to change your code to send the units as "% (bigger is better)" and we could add that here instead. https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:98: "why": "Used in dromaeo. Higher runs/ms implies faster execution." On 2014/01/09 03:47:43, tonyg wrote: > Not sure this one is used in dromaeo. Is it? Ah, that's right. It's not used in dromaeo. rileya added this about 2 weeks ago for "vector math" microbenchmarks. Will change comment. https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:104: "runs_per_s": { On 2014/01/09 03:47:43, tonyg wrote: > Any chance of getting the cros bots to use a canonical unit or should we just > bite the bullet and add this? I'm asking quickly why the cros bots use this unit. If it's simple/fast to change the units they send, then we shall just do that and not add this.
https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.json File tools/perf/unit-info.json (right): https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:82: "percentage": { On 2014/01/09 18:07:38, qyearsley wrote: > On 2014/01/09 04:42:11, victorhsieh wrote: > > On 2014/01/09 03:47:43, tonyg wrote: > > > Wondering if alloy should use a canonical unit instead of adding. > > > > No objection to use canonical unit, but in our case, it's a "up" (the higher > the > > better). > > > > Please let me know how should we handle the transition from our code, if the > > decision is to change this. Thanks. > > Alright, since the improvement direction is different I think it's fine to call > it "percentage" and have a comment that higher is better. > > Or, Victor, if you think it makes more sense and you want to change your code to > send the units as "% (bigger is better)" and we could add that here instead. Or "coverage %", whichever is more reusable. I actually don't know the unit does mean something. I though it's just a label until this code review. So just curious, how is the direction effect the dashboard? Also, if we change the unit, would it affect anything, since each data point has it own unit before?
https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.json File tools/perf/unit-info.json (right): https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:82: "percentage": { On 2014/01/09 18:41:56, victorhsieh wrote: > On 2014/01/09 18:07:38, qyearsley wrote: > > On 2014/01/09 04:42:11, victorhsieh wrote: > > > On 2014/01/09 03:47:43, tonyg wrote: > > > > Wondering if alloy should use a canonical unit instead of adding. > > > > > > No objection to use canonical unit, but in our case, it's a "up" (the higher > > the > > > better). > > > > > > Please let me know how should we handle the transition from our code, if the > > > decision is to change this. Thanks. > > > > Alright, since the improvement direction is different I think it's fine to > call > > it "percentage" and have a comment that higher is better. > > > > Or, Victor, if you think it makes more sense and you want to change your code > to > > send the units as "% (bigger is better)" and we could add that here instead. > > Or "coverage %", whichever is more reusable. > > I actually don't know the unit does mean something. I though it's just a label > until this code review. So just curious, how is the direction effect the > dashboard? > > Also, if we change the unit, would it affect anything, since each data point has > it own unit before? "coverage %" makes sense to me. In the datastore model used by the perf dashboard, the unit information is kept with the test, not with each data point, even though units are generally uploaded with each data point. If the dashboard starts receiving points for a test with a new units string, then it will update the units for that test, which will retroactively affect all points for that test. Now, when the dashboard back-end detects an upward or downward jump in the values for some test, whether this jump is marked as an improvement or a regression is determined by the units. The dashboard reads this file (unit-info.json), and it determines the improvement direction for the test from the units and the information here. So, for example, for all tests that have the units "%", downward movement is considered an improvement, because it says so in this file. (Note: This way of doing things works, but it has a couple small disadvantages: 1. this file has to be kept updated, and 2. different tests with the same unit (e.g. "score", "%", "count") might have different improvement direction. In the future, we might want to consider changing all tests to report improvement direction in addition to units.)
LGTM with the coverage change as discussed. https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.json File tools/perf/unit-info.json (right): https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:82: "percentage": { Thanks for the clear explanation. I'll change the unit from our side once this is landed. BTW, how about "coverage%" (remove the white space), just to avoid string parsing trouble. On 2014/01/09 19:09:29, qyearsley wrote: > On 2014/01/09 18:41:56, victorhsieh wrote: > > On 2014/01/09 18:07:38, qyearsley wrote: > > > On 2014/01/09 04:42:11, victorhsieh wrote: > > > > On 2014/01/09 03:47:43, tonyg wrote: > > > > > Wondering if alloy should use a canonical unit instead of adding. > > > > > > > > No objection to use canonical unit, but in our case, it's a "up" (the > higher > > > the > > > > better). > > > > > > > > Please let me know how should we handle the transition from our code, if > the > > > > decision is to change this. Thanks. > > > > > > Alright, since the improvement direction is different I think it's fine to > > call > > > it "percentage" and have a comment that higher is better. > > > > > > Or, Victor, if you think it makes more sense and you want to change your > code > > to > > > send the units as "% (bigger is better)" and we could add that here instead. > > > > Or "coverage %", whichever is more reusable. > > > > I actually don't know the unit does mean something. I though it's just a > label > > until this code review. So just curious, how is the direction effect the > > dashboard? > > > > Also, if we change the unit, would it affect anything, since each data point > has > > it own unit before? > > "coverage %" makes sense to me. > > In the datastore model used by the perf dashboard, the unit information is kept > with the test, not with each data point, even though units are generally > uploaded with each data point. If the dashboard starts receiving points for a > test with a new units string, then it will update the units for that test, which > will retroactively affect all points for that test. > > Now, when the dashboard back-end detects an upward or downward jump in the > values for some test, whether this jump is marked as an improvement or a > regression is determined by the units. > > The dashboard reads this file (unit-info.json), and it determines the > improvement direction for the test from the units and the information here. So, > for example, for all tests that have the units "%", downward movement is > considered an improvement, because it says so in this file. > > (Note: This way of doing things works, but it has a couple small disadvantages: > 1. this file has to be kept updated, and 2. different tests with the same unit > (e.g. "score", "%", "count") might have different improvement direction. In the > future, we might want to consider changing all tests to report improvement > direction in addition to units.)
https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.json File tools/perf/unit-info.json (right): https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:82: "percentage": { On 2014/01/09 19:16:48, victorhsieh wrote: > Thanks for the clear explanation. I'll change the unit from our side once this > is landed. > > BTW, how about "coverage%" (remove the white space), just to avoid string > parsing trouble. > > On 2014/01/09 19:09:29, qyearsley wrote: > > On 2014/01/09 18:41:56, victorhsieh wrote: > > > On 2014/01/09 18:07:38, qyearsley wrote: > > > > On 2014/01/09 04:42:11, victorhsieh wrote: > > > > > On 2014/01/09 03:47:43, tonyg wrote: > > > > > > Wondering if alloy should use a canonical unit instead of adding. > > > > > > > > > > No objection to use canonical unit, but in our case, it's a "up" (the > > higher > > > > the > > > > > better). > > > > > > > > > > Please let me know how should we handle the transition from our code, if > > the > > > > > decision is to change this. Thanks. > > > > > > > > Alright, since the improvement direction is different I think it's fine to > > > call > > > > it "percentage" and have a comment that higher is better. > > > > > > > > Or, Victor, if you think it makes more sense and you want to change your > > code > > > to > > > > send the units as "% (bigger is better)" and we could add that here > instead. > > > > > > Or "coverage %", whichever is more reusable. > > > > > > I actually don't know the unit does mean something. I though it's just a > > label > > > until this code review. So just curious, how is the direction effect the > > > dashboard? > > > > > > Also, if we change the unit, would it affect anything, since each data point > > has > > > it own unit before? > > > > "coverage %" makes sense to me. > > > > In the datastore model used by the perf dashboard, the unit information is > kept > > with the test, not with each data point, even though units are generally > > uploaded with each data point. If the dashboard starts receiving points for a > > test with a new units string, then it will update the units for that test, > which > > will retroactively affect all points for that test. > > > > Now, when the dashboard back-end detects an upward or downward jump in the > > values for some test, whether this jump is marked as an improvement or a > > regression is determined by the units. > > > > The dashboard reads this file (unit-info.json), and it determines the > > improvement direction for the test from the units and the information here. > So, > > for example, for all tests that have the units "%", downward movement is > > considered an improvement, because it says so in this file. > > > > (Note: This way of doing things works, but it has a couple small > disadvantages: > > 1. this file has to be kept updated, and 2. different tests with the same unit > > (e.g. "score", "%", "count") might have different improvement direction. In > the > > future, we might want to consider changing all tests to report improvement > > direction in addition to units.) > Sure, no problem. "%coverage" would also be fine. I'm pretty sure that the space character wouldn't cause an issue anywhere on the dashboard (units can be any string), but if it might cause an issue somewhere else, it would be fine to not have it. By the way, what is cts? And what is "alloy-perf-test/cts%/passed"? This is a good time to mention: There's also a "test-info.json" and "trace-info.json" file in the same directory as this file, which contain helpful little descriptions of test suites and individual trace lines on the graph. (These work similarly to unit-json and have the same disadvantage -- the dashboard reads these files and updates test information based on them.) Right now there are no descriptions for "alloy-perf-test" and "passed" in these files... could you describe what these things are? I actually don't know anything about Alloy, and I don't know where the test source code is located either...
https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.json File tools/perf/unit-info.json (right): https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:82: "percentage": { Unfortunately I can't mention the detail publicly. If you need the description for reference, would you please just mention that it's an internal experimental project, and maybe put my ldap as contact? On 2014/01/09 19:30:58, qyearsley wrote: > On 2014/01/09 19:16:48, victorhsieh wrote: > > Thanks for the clear explanation. I'll change the unit from our side once > this > > is landed. > > > > BTW, how about "coverage%" (remove the white space), just to avoid string > > parsing trouble. > > > > On 2014/01/09 19:09:29, qyearsley wrote: > > > On 2014/01/09 18:41:56, victorhsieh wrote: > > > > On 2014/01/09 18:07:38, qyearsley wrote: > > > > > On 2014/01/09 04:42:11, victorhsieh wrote: > > > > > > On 2014/01/09 03:47:43, tonyg wrote: > > > > > > > Wondering if alloy should use a canonical unit instead of adding. > > > > > > > > > > > > No objection to use canonical unit, but in our case, it's a "up" (the > > > higher > > > > > the > > > > > > better). > > > > > > > > > > > > Please let me know how should we handle the transition from our code, > if > > > the > > > > > > decision is to change this. Thanks. > > > > > > > > > > Alright, since the improvement direction is different I think it's fine > to > > > > call > > > > > it "percentage" and have a comment that higher is better. > > > > > > > > > > Or, Victor, if you think it makes more sense and you want to change your > > > code > > > > to > > > > > send the units as "% (bigger is better)" and we could add that here > > instead. > > > > > > > > Or "coverage %", whichever is more reusable. > > > > > > > > I actually don't know the unit does mean something. I though it's just a > > > label > > > > until this code review. So just curious, how is the direction effect the > > > > dashboard? > > > > > > > > Also, if we change the unit, would it affect anything, since each data > point > > > has > > > > it own unit before? > > > > > > "coverage %" makes sense to me. > > > > > > In the datastore model used by the perf dashboard, the unit information is > > kept > > > with the test, not with each data point, even though units are generally > > > uploaded with each data point. If the dashboard starts receiving points for > a > > > test with a new units string, then it will update the units for that test, > > which > > > will retroactively affect all points for that test. > > > > > > Now, when the dashboard back-end detects an upward or downward jump in the > > > values for some test, whether this jump is marked as an improvement or a > > > regression is determined by the units. > > > > > > The dashboard reads this file (unit-info.json), and it determines the > > > improvement direction for the test from the units and the information here. > > So, > > > for example, for all tests that have the units "%", downward movement is > > > considered an improvement, because it says so in this file. > > > > > > (Note: This way of doing things works, but it has a couple small > > disadvantages: > > > 1. this file has to be kept updated, and 2. different tests with the same > unit > > > (e.g. "score", "%", "count") might have different improvement direction. In > > the > > > future, we might want to consider changing all tests to report improvement > > > direction in addition to units.) > > > > Sure, no problem. "%coverage" would also be fine. > > I'm pretty sure that the space character wouldn't cause an issue anywhere on the > dashboard (units can be any string), but if it might cause an issue somewhere > else, it would be fine to not have it. > > By the way, what is cts? And what is "alloy-perf-test/cts%/passed"? > > This is a good time to mention: There's also a "test-info.json" and > "trace-info.json" file in the same directory as this file, which contain helpful > little descriptions of test suites and individual trace lines on the graph. > (These work similarly to unit-json and have the same disadvantage -- the > dashboard reads these files and updates test information based on them.) > > Right now there are no descriptions for "alloy-perf-test" and "passed" in these > files... could you describe what these things are? I actually don't know > anything about Alloy, and I don't know where the test source code is located > either...
https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.json File tools/perf/unit-info.json (right): https://codereview.chromium.org/130613002/diff/60001/tools/perf/unit-info.jso... tools/perf/unit-info.json:82: "percentage": { Ah, if it's internal that would explain why I couldn't find it in the main chromium source tree. No problem. On 2014/01/09 19:44:42, victorhsieh wrote: > Unfortunately I can't mention the detail publicly. If you need the description > for reference, would you please just mention that it's an internal experimental > project, and maybe put my ldap as contact?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qyearsley@chromium.org/130613002/200001
On 2014/01/11 02:21:58, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/qyearsley%40chromium.org/130613002/200001 Note: I found out why "runs_per_s" is used for the units sent by the ChromeOS bots -- there's actually a function that's being called which replaces "/" with "_per_". According to the comment above that function, the "/" is "illegal"... https://cs.corp.google.com/p#chromeos_public/src/third_party/autotest/files/s...
Message was sent while issue was closed.
Change committed as 244390 |
