Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2718743002:
📼 Use non-deprecated arguments for resource_sizes.py (Closed)
DescriptionUse non-deprecated arguments for resource_sizes.py
Patch Set 1 #
Total comments: 3
Patch Set 2 : parse apk name from suite name #Patch Set 3 : stories #Messages
Total messages: 11 (2 generated)
agrieve@chromium.org changed reviewers: + sullivan@chromium.org
https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... File dashboard/dashboard/start_try_job.py (right): https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... dashboard/dashboard/start_try_job.py:84: 'src/out/Release/apks/Chrome.apk', not actually sure what this is used for (just found it via grep). Note that we alert on MonochromePublic.apk, so maybe that would be better than Chrome.apk?
sullivan@chromium.org changed reviewers: + simonhatch@chromium.org
Simon, can you take a look?
So whenever I have a few minutes here and there, I've made a few half-hearted attempts to get resource_sizes working again. Here's the dashboard side you'll need to at least run it: https://codereview.chromium.org/2707383009/ To test this you need to stand up a dashboard and then kick off a bisect on the staging server (nexus5x) (or I can do it by patching in your cl). https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... File dashboard/dashboard/start_try_job.py (right): https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... dashboard/dashboard/start_try_job.py:84: 'src/out/Release/apks/Chrome.apk', On 2017/02/24 19:50:10, agrieve wrote: > not actually sure what this is used for (just found it via grep). Note that we > alert on MonochromePublic.apk, so maybe that would be better than Chrome.apk? From the looks of resource_size.py it takes the path to an apk to produce the various metrics, so seemingly we'd want to to pass that in. If we want to be proper, we should actually parse it from the passed in suite name, which is going to be "resource_sizes (MonochromePublic.apk)" in case anything else ever gets monitored.
On 2017/02/24 21:58:32, shatch wrote: > So whenever I have a few minutes here and there, I've made a few half-hearted > attempts to get resource_sizes working again. > > Here's the dashboard side you'll need to at least run it: > https://codereview.chromium.org/2707383009/ > > To test this you need to stand up a dashboard and then kick off a bisect on the > staging server (nexus5x) (or I can do it by patching in your cl). > > https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... > File dashboard/dashboard/start_try_job.py (right): > > https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... > dashboard/dashboard/start_try_job.py:84: 'src/out/Release/apks/Chrome.apk', > On 2017/02/24 19:50:10, agrieve wrote: > > not actually sure what this is used for (just found it via grep). Note that we > > alert on MonochromePublic.apk, so maybe that would be better than Chrome.apk? > > From the looks of resource_size.py it takes the path to an apk to produce the > various metrics, so seemingly we'd want to to pass that in. If we want to be > proper, we should actually parse it from the passed in suite name, which is > going to be "resource_sizes (MonochromePublic.apk)" in case anything else ever > gets monitored. My intention here was just to remove the reference to "--so-with-symbols-path". If this is all it takes to fix bisects though, then that's great!. I'd be thrilled if you'd like to test out this change via patching, but if there are instructions somewhere about how to set up a server, I'm also willing to go that route.
https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... File dashboard/dashboard/start_try_job.py (right): https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... dashboard/dashboard/start_try_job.py:84: 'src/out/Release/apks/Chrome.apk', On 2017/02/24 21:58:31, shatch wrote: > On 2017/02/24 19:50:10, agrieve wrote: > > not actually sure what this is used for (just found it via grep). Note that we > > alert on MonochromePublic.apk, so maybe that would be better than Chrome.apk? > > From the looks of resource_size.py it takes the path to an apk to produce the > various metrics, so seemingly we'd want to to pass that in. If we want to be > proper, we should actually parse it from the passed in suite name, which is > going to be "resource_sizes (MonochromePublic.apk)" in case anything else ever > gets monitored. Done.
On 2017/02/27 18:56:52, agrieve wrote: > https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... > File dashboard/dashboard/start_try_job.py (right): > > https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... > dashboard/dashboard/start_try_job.py:84: 'src/out/Release/apks/Chrome.apk', > On 2017/02/24 21:58:31, shatch wrote: > > On 2017/02/24 19:50:10, agrieve wrote: > > > not actually sure what this is used for (just found it via grep). Note that > we > > > alert on MonochromePublic.apk, so maybe that would be better than > Chrome.apk? > > > > From the looks of resource_size.py it takes the path to an apk to produce the > > various metrics, so seemingly we'd want to to pass that in. If we want to be > > proper, we should actually parse it from the passed in suite name, which is > > going to be "resource_sizes (MonochromePublic.apk)" in case anything else ever > > gets monitored. > > Done. Just to update, I patched in the CL and sent a test bisect but it failed with an error about the path to the apk. May need to fill that in recipe side which isn't too hard, but the staging servers are down at the moment (crbug.com/697159) so I'm waiting for those to come back online.
On 2017/03/01 23:17:41, shatch wrote: > On 2017/02/27 18:56:52, agrieve wrote: > > > https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... > > File dashboard/dashboard/start_try_job.py (right): > > > > > https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... > > dashboard/dashboard/start_try_job.py:84: 'src/out/Release/apks/Chrome.apk', > > On 2017/02/24 21:58:31, shatch wrote: > > > On 2017/02/24 19:50:10, agrieve wrote: > > > > not actually sure what this is used for (just found it via grep). Note > that > > we > > > > alert on MonochromePublic.apk, so maybe that would be better than > > Chrome.apk? > > > > > > From the looks of resource_size.py it takes the path to an apk to produce > the > > > various metrics, so seemingly we'd want to to pass that in. If we want to be > > > proper, we should actually parse it from the passed in suite name, which is > > > going to be "resource_sizes (MonochromePublic.apk)" in case anything else > ever > > > gets monitored. > > > > Done. > > Just to update, I patched in the CL and sent a test bisect but it failed with an > error about the path to the apk. May need to fill that in recipe side which > isn't too hard, but the staging servers are down at the moment > (crbug.com/697159) so I'm waiting for those to come back online. Hurray we have success! https://build.chromium.org/p/tryserver.chromium.perf/builders/staging_android... Had to make some minor changes to this CL: https://codereview.chromium.org/2734773003/ Also, MonochromePublic.apk isn't built by default for the perf targets, so we'll need to get that included in the build as well.
On 2017/03/07 01:43:06, shatch wrote: > On 2017/03/01 23:17:41, shatch wrote: > > On 2017/02/27 18:56:52, agrieve wrote: > > > > > > https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... > > > File dashboard/dashboard/start_try_job.py (right): > > > > > > > > > https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... > > > dashboard/dashboard/start_try_job.py:84: 'src/out/Release/apks/Chrome.apk', > > > On 2017/02/24 21:58:31, shatch wrote: > > > > On 2017/02/24 19:50:10, agrieve wrote: > > > > > not actually sure what this is used for (just found it via grep). Note > > that > > > we > > > > > alert on MonochromePublic.apk, so maybe that would be better than > > > Chrome.apk? > > > > > > > > From the looks of resource_size.py it takes the path to an apk to produce > > the > > > > various metrics, so seemingly we'd want to to pass that in. If we want to > be > > > > proper, we should actually parse it from the passed in suite name, which > is > > > > going to be "resource_sizes (MonochromePublic.apk)" in case anything else > > ever > > > > gets monitored. > > > > > > Done. > > > > Just to update, I patched in the CL and sent a test bisect but it failed with > an > > error about the path to the apk. May need to fill that in recipe side which > > isn't too hard, but the staging servers are down at the moment > > (crbug.com/697159) so I'm waiting for those to come back online. > > Hurray we have success! > > https://build.chromium.org/p/tryserver.chromium.perf/builders/staging_android... > > > Had to make some minor changes to this CL: > https://codereview.chromium.org/2734773003/ > > > Also, MonochromePublic.apk isn't built by default for the perf targets, so we'll > need to get that included in the build as well. Sweet deal! Shall I leave you to submit your working version? How do we get MonochromePublic.apk building?
On 2017/03/08 15:55:23, agrieve wrote: > On 2017/03/07 01:43:06, shatch wrote: > > On 2017/03/01 23:17:41, shatch wrote: > > > On 2017/02/27 18:56:52, agrieve wrote: > > > > > > > > > > https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... > > > > File dashboard/dashboard/start_try_job.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2718743002/diff/1/dashboard/dashboard/start_t... > > > > dashboard/dashboard/start_try_job.py:84: > 'src/out/Release/apks/Chrome.apk', > > > > On 2017/02/24 21:58:31, shatch wrote: > > > > > On 2017/02/24 19:50:10, agrieve wrote: > > > > > > not actually sure what this is used for (just found it via grep). Note > > > that > > > > we > > > > > > alert on MonochromePublic.apk, so maybe that would be better than > > > > Chrome.apk? > > > > > > > > > > From the looks of resource_size.py it takes the path to an apk to > produce > > > the > > > > > various metrics, so seemingly we'd want to to pass that in. If we want > to > > be > > > > > proper, we should actually parse it from the passed in suite name, which > > is > > > > > going to be "resource_sizes (MonochromePublic.apk)" in case anything > else > > > ever > > > > > gets monitored. > > > > > > > > Done. > > > > > > Just to update, I patched in the CL and sent a test bisect but it failed > with > > an > > > error about the path to the apk. May need to fill that in recipe side which > > > isn't too hard, but the staging servers are down at the moment > > > (crbug.com/697159) so I'm waiting for those to come back online. > > > > Hurray we have success! > > > > > https://build.chromium.org/p/tryserver.chromium.perf/builders/staging_android... > > > > > > Had to make some minor changes to this CL: > > https://codereview.chromium.org/2734773003/ > > > > > > Also, MonochromePublic.apk isn't built by default for the perf targets, so > we'll > > need to get that included in the build as well. > > Sweet deal! Shall I leave you to submit your working version? > How do we get MonochromePublic.apk building? Yeah I can put what I have up. Next steps are to get MonochromePublic.apk included with the perf builds (not totally sure where this is done but shouldn't be too hard to track down), and then roll my changes to the bisect recipe from staging to prod. Might take a bit of time to get to, bit swamped at the moment. |
