Issue 7322085: bench_graph_svg: HUGE speedup for parsing tiled bench data (Closed)
|
Created:
11 years, 10 months ago by epoger Modified:
11 years, 10 months ago CC:
skia-review_googlegroups.com, edisonn Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
Descriptionbench_graph_svg: HUGE speedup for parsing tiled bench data
Committed: https://code.google.com/p/skia/source/detail?r=7762
Patch Set 1 #
Total comments: 1
MessagesTotal messages: 16
Hey, I just met you, and this is crazy, But this runs 90% faster, so try it maybe? The bench_graph_svg.py test in tools/tests/run.sh dropped from 39 seconds to 3 seconds with this tiny change (suggested by Ben in https://codereview.appspot.com/7336043 )...
Sign in to reply to this message.
+1000
Sign in to reply to this message.
On 2013/02/15 17:04:54, EricB wrote: > +1000 ditto
Sign in to reply to this message.
On 2013/02/15 17:05:54, rmistry wrote: > On 2013/02/15 17:04:54, EricB wrote: > > +1000 > > ditto Also, can we please make a memegen about your initial comment.
Sign in to reply to this message.
On 2013/02/15 17:06:36, rmistry wrote: > On 2013/02/15 17:05:54, rmistry wrote: > > On 2013/02/15 17:04:54, EricB wrote: > > > +1000 > > > > ditto > > Also, can we please make a memegen about your initial comment. https://memegen.googleplex.com/15653779 Did I do that right?
Sign in to reply to this message.
I have no idea what that means but.. +1 !!!! On Fri, Feb 15, 2013 at 12:12 PM, <epoger@google.com> wrote: > On 2013/02/15 17:06:36, rmistry wrote: > >> On 2013/02/15 17:05:54, rmistry wrote: >> > On 2013/02/15 17:04:54, EricB wrote: >> > > +1000 >> > >> > ditto >> > > Also, can we please make a memegen about your initial comment. >> > > https://memegen.googleplex.**com/15653779<https://memegen.googleplex.com/1565... > > Did I do that right? > > https://codereview.appspot.**com/7322085/<https://codereview.appspot.com/7322... >
Sign in to reply to this message.
On 2013/02/15 17:13:54, rmistry wrote: > I have no idea what that means but.. +1 !!!! http://www.tumblr.com/tagged/there's%20always%20money%20in%20the%20banana%20s...
Sign in to reply to this message.
LGTM. Ditto on Ravi's comment on the meme. https://codereview.appspot.com/7322085/diff/1/bench/bench_util.py File bench/bench_util.py (right): https://codereview.appspot.com/7322085/diff/1/bench/bench_util.py#newcode181 bench/bench_util.py:181: bench_dic, layout_dic, representation) My only concern is whether this will screw things up when we need to turn off timeIndividualTiles again. If we add checking of ' out of ' that should take care of the case.
Sign in to reply to this message.
Thanks for the context. Means never give up on hope. BTW my suggestion meant to cut the time in half, but how come it cut >90% instead?... Shall we revert it until we fully understand it? (shunning rotten tomatoes) On 2013/02/15 17:18:35, benchen wrote: > LGTM. Ditto on Ravi's comment on the meme. > > https://codereview.appspot.com/7322085/diff/1/bench/bench_util.py > File bench/bench_util.py (right): > > https://codereview.appspot.com/7322085/diff/1/bench/bench_util.py#newcode181 > bench/bench_util.py:181: bench_dic, layout_dic, representation) > My only concern is whether this will screw things up when we need to turn off > timeIndividualTiles again. If we add checking of ' out of ' that should take > care of the case.
Sign in to reply to this message.
On 2013/02/15 17:25:49, benchen wrote: > Thanks for the context. Means never give up on hope. > BTW my suggestion meant to cut the time in half, but how come it cut >90% > instead?... Shall we revert it until we fully understand it? (shunning rotten > tomatoes) > > On 2013/02/15 17:18:35, benchen wrote: > > LGTM. Ditto on Ravi's comment on the meme. > > > > https://codereview.appspot.com/7322085/diff/1/bench/bench_util.py > > File bench/bench_util.py (right): > > > > https://codereview.appspot.com/7322085/diff/1/bench/bench_util.py#newcode181 > > bench/bench_util.py:181: bench_dic, layout_dic, representation) > > My only concern is whether this will screw things up when we need to turn off > > timeIndividualTiles again. If we add checking of ' out of ' that should take > > care of the case. OK... I've gotten confused about what's on and what's off... here's my understanding: - As of http://code.google.com/p/skia/source/detail?r=7620 , per-TILE reporting (--timeIndividualTiles) is turned ON. - As of http://code.google.com/p/skia/source/detail?r=7691 , per-ITERATION tile time reporting is turned OFF. As verification: looking at http://chromium-skia-gm.commondatastorage.googleapis.com/playback/perfdata/Sk... , I see per-tile times, but only as AVERAGES. To allay this concern, should we add data generated WITHOUT --timeIndividualTiles to the self-test? Would http://chromium-skia-gm.commondatastorage.googleapis.com/playback/perfdata/Sk... suffice for that?
Sign in to reply to this message.
On 2013/02/15 17:53:54, epoger wrote: > On 2013/02/15 17:25:49, benchen wrote: > > Thanks for the context. Means never give up on hope. > > BTW my suggestion meant to cut the time in half, but how come it cut >90% > > instead?... Shall we revert it until we fully understand it? (shunning rotten > > tomatoes) > > > > On 2013/02/15 17:18:35, benchen wrote: > > > LGTM. Ditto on Ravi's comment on the meme. > > > > > > https://codereview.appspot.com/7322085/diff/1/bench/bench_util.py > > > File bench/bench_util.py (right): > > > > > > https://codereview.appspot.com/7322085/diff/1/bench/bench_util.py#newcode181 > > > bench/bench_util.py:181: bench_dic, layout_dic, representation) > > > My only concern is whether this will screw things up when we need to turn > off > > > timeIndividualTiles again. If we add checking of ' out of ' that should take > > > care of the case. > > OK... I've gotten confused about what's on and what's off... here's my > understanding: > > - As of http://code.google.com/p/skia/source/detail?r=7620 , per-TILE reporting > (--timeIndividualTiles) is turned ON. > > - As of http://code.google.com/p/skia/source/detail?r=7691 , per-ITERATION tile > time reporting is turned OFF. > > As verification: looking at > http://chromium-skia-gm.commondatastorage.googleapis.com/playback/perfdata/Sk... > , I see per-tile times, but only as AVERAGES. > > To allay this concern, should we add data generated WITHOUT > --timeIndividualTiles to the self-test? Would > http://chromium-skia-gm.commondatastorage.googleapis.com/playback/perfdata/Sk... > suffice for that? Looks like my concern is unnecessary - when we don't log per tile, the tile_* config name won't be in a separate line. We are logging per-tile but not per-iter of each tile, which is desired. If per-iter were added back they would be ignored in the current codes. Just in case, I think adding the tile_ config data without individual tiles will be more future-proof. The r7618 data looks fine. Thanks!
Sign in to reply to this message.
On 2013/02/15 18:00:48, benchen wrote: > Looks like my concern is unnecessary - when we don't log per tile, the tile_* > config name won't be in a separate line. > We are logging per-tile but not per-iter of each tile, which is desired. If > per-iter were added back they would be ignored in the current codes. > Just in case, I think adding the tile_ config data without individual tiles will > be more future-proof. The r7618 data looks fine. Thanks! Paused this CL, pending https://codereview.appspot.com/7305098/ ('bench_graph_svg: add r7618 results (NOT per-tile) to self-test source data')
Sign in to reply to this message.
On 2013/02/15 18:28:19, epoger wrote: > Paused this CL, pending https://codereview.appspot.com/7305098/ > ('bench_graph_svg: add r7618 results (NOT per-tile) to self-test source data') Still works after the commit of https://codereview.appspot.com/7305098/ ! Committing...
Sign in to reply to this message.
Message was sent while issue was closed.
bench_graph_svg self-test results from the production housekeeping bot: BEFORE: 70 seconds http://70.32.156.53:10117/builders/Skia_PerCommit_House_Keeping/builds/2100/s... AFTER: 3 seconds http://70.32.156.53:10117/builders/Skia_PerCommit_House_Keeping/builds/2101/s... :-) And I'm sure there is plenty of fat left to be cut out... (although I can't imagine finding more wins this big!) Now, let's see how GenerateWebpagePictureBenchResults fares on the bench bots...
Sign in to reply to this message.
Message was sent while issue was closed.
The results are in from the first real bench bot... they're not nearly as dramatic as the self-test. But 52% faster is still a nice win, and I'm sure there is plenty more fat to cut out. BEFORE: 337 seconds http://70.32.156.53:10117/builders/Skia_Shuttle_Ubuntu12_ATI5770_Float_Bench_... AFTER: 163 seconds http://70.32.156.53:10117/builders/Skia_Shuttle_Ubuntu12_ATI5770_Float_Bench_... SO... Ben... can you go in there and look for more fat to cut out?
Sign in to reply to this message.
Cannot think of any other idea for now. If this is really related to RE, my only other option would be to get rid of all RE's. That'll bring us back to Edi's JSON bench side-output work. ben On Feb 17, 2013 4:51 AM, <epoger@google.com> wrote: > The results are in from the first real bench bot... they're not nearly > as dramatic as the self-test. But 52% faster is still a nice win, and > I'm sure there is plenty more fat to cut out. > > BEFORE: 337 seconds > http://70.32.156.53:10117/**builders/Skia_Shuttle_** > Ubuntu12_ATI5770_Float_Bench_**64/builds/958<http://70.32.156.53:10117/builders/Skia_Shuttle_Ubuntu12_ATI5770_Float_Bench_64/builds/958> > > AFTER: 163 seconds > http://70.32.156.53:10117/**builders/Skia_Shuttle_** > Ubuntu12_ATI5770_Float_Bench_**64/builds/959<http://70.32.156.53:10117/builders/Skia_Shuttle_Ubuntu12_ATI5770_Float_Bench_64/builds/959> > > SO... Ben... can you go in there and look for more fat to cut out? > > https://codereview.appspot.**com/7322085/<https://codereview.appspot.com/7322... >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
