Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2651623005:
[tools/perf] Create a list of tags for system_health_story (Closed)
Description[tools/perf] Create a list of tags for system_health_story
BUG=
Review-Url: https://codereview.chromium.org/2651623005
Cr-Commit-Position: refs/heads/master@{#448026}
Committed: https://chromium.googlesource.com/chromium/src/+/6a67824709b57582fc3957f25b5abad81a90a324
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address review comments #
Total comments: 16
Patch Set 3 : Address review comments #Patch Set 4 : Address review comments #
Messages
Total messages: 26 (12 generated)
nednguyen@google.com changed reviewers: + charliea@chromium.org, perezju@chromium.org, rnephew@chromium.org
https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_tags.py (right): https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:3: # found in the LICENSE file. Note that I have name this "system_health_tags" instead of just "tags" to avoid naming collision with the tags variable.
lgtm with a nit and 2 suggestions. https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_tags.py (right): https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:3: # found in the LICENSE file. On 2017/01/24 17:59:01, nednguyen wrote: > Note that I have name this "system_health_tags" instead of just "tags" to avoid > naming collision with the tags variable. If this plans to go farther than system health benchmarks maybe benchmark_tags would be better? https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:24: 'canvas-animation', 'stories with this tags have animations that are ' Nit:Inconsistent capitalization between tags. https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:59: for obj in dir(sys.modules[__name__]): A comment on what this is actually doing would be nice, wasn't immediately obvious.
https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_story.py (right): https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_story.py:77: tags.append(t) High level question, as a benchmark writer you expect clients to write e.g.: class MyNewFunStory(system_health_story.SystemHealthStory): TAGS = ['audio-playing', 'css-animation'] ... And then this bit of code here just checks that the tags provided (as strings) are valid? If that is the case (and I think that's a good interface), maybe just expose a system_health_tags.ValidateTags() method? No need to define a TagInfo class at all, a simple private dict mapping tags to their descriptions should be enough. What do you think? https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_tags.py (right): https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:22: 'audio-playing', 'Stories with this tag have audio playing') "Stories with this tag have .." an all descriptions is a bit repetitive and doesn't add much info. Maybe: "Story plays audio", "Story has html5 canvas animations", "Story has CSS animations", etc.? Also make them full sentences (i.e. start uppercase and end with a full stop.)
https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_story.py (right): https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_story.py:73: tags = [] Why build up tags? Can't we just pass in self.TAGS after validating that they all belong to ALL_TAGS? https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_tags.py (right): https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:3: # found in the LICENSE file. On 2017/01/24 18:23:09, rnephew (Reviews Here) wrote: > On 2017/01/24 17:59:01, nednguyen wrote: > > Note that I have name this "system_health_tags" instead of just "tags" to > avoid > > naming collision with the tags variable. > > If this plans to go farther than system health benchmarks maybe benchmark_tags > would be better? My vote for the name is story_tags because: - system_health.system_health_tags is redundant - system_health.benchmark_tags indicates that the tags apply to the benchmarks, but they actually apply to the stories https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:22: 'audio-playing', 'Stories with this tag have audio playing') It seems like we should be consistent about what's in a tag name: if we have "audio playing" (present participle), then it should be "canvas-animating" (present participle) instead of "canvas animation" (noun). My vote is for all nouns: audio, canvas-animation, css-animation, pinch-zoom, images, multi-tabs, international, scroll, pinch-zoom, multi-tabs, video, webgl, web-storage only changes required are: audio-playing =>audio, video-playing=>video https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:33: 'web-storage', 'stories with this tags have sites with heavy uses of ' wut? web-storage == images? https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:36: 'multi-tabs', 'stories with this tags include navigating to websites ' same https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:59: for obj in dir(sys.modules[__name__]): On 2017/01/24 18:23:09, rnephew (Reviews Here) wrote: > A comment on what this is actually doing would be nice, wasn't immediately > obvious. +1
https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_tags.py (right): https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:22: 'audio-playing', 'Stories with this tag have audio playing') On 2017/01/24 23:00:05, charliea wrote: > It seems like we should be consistent about what's in a tag name: if we have > "audio playing" (present participle), then it should be "canvas-animating" > (present participle) instead of "canvas animation" (noun). > > My vote is for all nouns: > > audio, canvas-animation, css-animation, pinch-zoom, images, multi-tabs, > international, scroll, pinch-zoom, multi-tabs, video, webgl, web-storage > > only changes required are: > > audio-playing =>audio, video-playing=>video > +1 to all nouns, and document this fact on the file. I would suggest, though: audio-playback, video-playback
https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_story.py (right): https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_story.py:73: tags = [] On 2017/01/24 23:00:05, charliea (OOOish - BlinkOn) wrote: > Why build up tags? Can't we just pass in self.TAGS after validating that they > all belong to ALL_TAGS? Actually the TAG_INFOS, not TAGS, so people can write: TAG_INFOS = [story_tags.AUDIO_PLAYING, story_tags.CSS_ANIMATION] https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_story.py:77: tags.append(t) On 2017/01/24 19:50:05, perezju wrote: > High level question, as a benchmark writer you expect clients to write e.g.: > > class MyNewFunStory(system_health_story.SystemHealthStory): > TAGS = ['audio-playing', 'css-animation'] > ... > > And then this bit of code here just checks that the tags provided (as strings) > are valid? > > If that is the case (and I think that's a good interface), maybe just expose a > system_health_tags.ValidateTags() method? > > No need to define a TagInfo class at all, a simple private dict mapping tags to > their descriptions should be enough. What do you think? I expect clients to write: TAGS = [story_tags.AUDIO_PLAYING, story_tags.CSS_ANIMATION] Using a dict as you proposed make it a bit clumsy for ensuring documentation for each extra tag, i.e: TAG_A = '...' TAG_B = '...' .... description = { TAG_A: '...', # <- adding another tag require adding code in two separate place TAG_B: '...', ... } https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_tags.py (right): https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:3: # found in the LICENSE file. On 2017/01/24 23:00:05, charliea (OOOish - BlinkOn) wrote: > On 2017/01/24 18:23:09, rnephew (Reviews Here) wrote: > > On 2017/01/24 17:59:01, nednguyen wrote: > > > Note that I have name this "system_health_tags" instead of just "tags" to > > avoid > > > naming collision with the tags variable. > > > > If this plans to go farther than system health benchmarks maybe benchmark_tags > > would be better? > > My vote for the name is story_tags because: > > - system_health.system_health_tags is redundant > - system_health.benchmark_tags indicates that the tags apply to the > benchmarks, but they actually apply to the stories Done. https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:22: 'audio-playing', 'Stories with this tag have audio playing') On 2017/01/25 00:06:45, perezju wrote: > On 2017/01/24 23:00:05, charliea wrote: > > It seems like we should be consistent about what's in a tag name: if we have > > "audio playing" (present participle), then it should be "canvas-animating" > > (present participle) instead of "canvas animation" (noun). > > > > My vote is for all nouns: > > > > audio, canvas-animation, css-animation, pinch-zoom, images, multi-tabs, > > international, scroll, pinch-zoom, multi-tabs, video, webgl, web-storage > > > > only changes required are: > > > > audio-playing =>audio, video-playing=>video > > > > +1 to all nouns, and document this fact on the file. > > I would suggest, though: audio-playback, video-playback Done. https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:24: 'canvas-animation', 'stories with this tags have animations that are ' On 2017/01/24 18:23:09, rnephew (Reviews Here) wrote: > Nit:Inconsistent capitalization between tags. Done. https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:33: 'web-storage', 'stories with this tags have sites with heavy uses of ' On 2017/01/24 23:00:05, charliea (OOOish - BlinkOn) wrote: > wut? web-storage == images? Done. https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:36: 'multi-tabs', 'stories with this tags include navigating to websites ' On 2017/01/24 23:00:05, charliea (OOOish - BlinkOn) wrote: > same Done. https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_tags.py:59: for obj in dir(sys.modules[__name__]): On 2017/01/24 23:00:05, charliea (OOOish - BlinkOn) wrote: > On 2017/01/24 18:23:09, rnephew (Reviews Here) wrote: > > A comment on what this is actually doing would be nice, wasn't immediately > > obvious. > > +1 Done.
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/system_health_story.py (right): https://codereview.chromium.org/2651623005/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/system_health_story.py:77: tags.append(t) On 2017/02/03 15:02:42, nednguyen wrote: > On 2017/01/24 19:50:05, perezju wrote: > > High level question, as a benchmark writer you expect clients to write e.g.: > > > > class MyNewFunStory(system_health_story.SystemHealthStory): > > TAGS = ['audio-playing', 'css-animation'] > > ... > > > > And then this bit of code here just checks that the tags provided (as strings) > > are valid? > > > > If that is the case (and I think that's a good interface), maybe just expose a > > system_health_tags.ValidateTags() method? > > > > No need to define a TagInfo class at all, a simple private dict mapping tags > to > > their descriptions should be enough. What do you think? > > I expect clients to write: > TAGS = [story_tags.AUDIO_PLAYING, story_tags.CSS_ANIMATION] > > Using a dict as you proposed make it a bit clumsy for ensuring documentation for > each extra tag, i.e: > > TAG_A = '...' > TAG_B = '...' > > .... > > description = { > TAG_A: '...', # <- adding another tag require adding code in two separate place > TAG_B: '...', > ... > } Acknowledged. If you wan't to define (and use) the constant names then a dictionary doesn't make sense. https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:8: class TagInfo(object): why not make this a collections.namedtuple? also, an opinion nit, I don't like much the distinction between "tags" and "tag infos"; I could call this class "Tag" with a "name" property instead, the module can then expose an ALL_TAGS list, while story classes define the TAGS they use. https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:26: 'audio-playback', 'Story has audio playing') nit: consistency, end all sentences with a period. https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:33: 'pinch-zoom', 'Story has browser with extension installed') pinch-zoom? https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:38: 'speaking countries.') nit: ... to websites with content in non English languages. (I guess what matters is the language of the content, not what people speak in that country.) https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:44: 'multi-tabs', 'Story has multi tabs and tabs switching action') nit: call this TAB_SWITCHING and 'tab-switching' ? https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:61: assert obj.tag not in all_tags, 'Duplicate tag: %s' % obj.tag Maybe move this to a unit test? I think this would actually fail as it is now (because of the mistake of having 'pinch-zoom' twice), and it would be good to catch this while running tests rather than on live code.
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:8: class TagInfo(object): On 2017/02/03 15:52:21, perezju wrote: > why not make this a collections.namedtuple? > > also, an opinion nit, I don't like much the distinction between "tags" and "tag > infos"; I could call this class "Tag" with a "name" property instead, the module > can then expose an ALL_TAGS list, while story classes define the TAGS they use. collections.namedtuple is mutable, so I prefer using @property to make it "immutable". Rename it to Tag sgtm https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:26: 'audio-playback', 'Story has audio playing') On 2017/02/03 15:52:21, perezju wrote: > nit: consistency, end all sentences with a period. Done. https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:33: 'pinch-zoom', 'Story has browser with extension installed') On 2017/02/03 15:52:21, perezju wrote: > pinch-zoom? Done. https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:38: 'speaking countries.') On 2017/02/03 15:52:22, perezju wrote: > nit: ... to websites with content in non English languages. > > (I guess what matters is the language of the content, not what people speak in > that country.) Done. https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:44: 'multi-tabs', 'Story has multi tabs and tabs switching action') On 2017/02/03 15:52:21, perezju wrote: > nit: call this TAB_SWITCHING and 'tab-switching' ? Done. https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:61: assert obj.tag not in all_tags, 'Duplicate tag: %s' % obj.tag On 2017/02/03 15:52:21, perezju wrote: > Maybe move this to a unit test? > > I think this would actually fail as it is now (because of the mistake of having > 'pinch-zoom' twice), and it would be good to catch this while running tests > rather than on live code. I think when there is inconsistent/wrong logic, it's better to fail hard rather than having flawed output, especially for non end-users facing products like benchmarks.
https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:8: class TagInfo(object): On 2017/02/03 16:07:55, nednguyen wrote: > On 2017/02/03 15:52:21, perezju wrote: > > why not make this a collections.namedtuple? > > > > also, an opinion nit, I don't like much the distinction between "tags" and > "tag > > infos"; I could call this class "Tag" with a "name" property instead, the > module > > can then expose an ALL_TAGS list, while story classes define the TAGS they > use. > > collections.namedtuple is mutable, so I prefer using @property to make it > "immutable". Rename it to Tag sgtm No, it's not. It's a tuple subclass, and therefore not mutable. >>> import collections >>> Tag = collections.namedtuple('Tag', ['name', 'description']) >>> t = Tag('foo', 'bar') >>> t.name = 'nope' Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: can't set attribute >>> t Tag(name='foo', description='bar') >>> https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:61: assert obj.tag not in all_tags, 'Duplicate tag: %s' % obj.tag On 2017/02/03 16:07:55, nednguyen wrote: > On 2017/02/03 15:52:21, perezju wrote: > > Maybe move this to a unit test? > > > > I think this would actually fail as it is now (because of the mistake of > having > > 'pinch-zoom' twice), and it would be good to catch this while running tests > > rather than on live code. > > I think when there is inconsistent/wrong logic, it's better to fail hard rather > than having flawed output, especially for non end-users facing products like > benchmarks. Yes, agree. What I was trying to say is that this should *also* fail on CQ. Not sure why it passed all your try jobs above (maybe because the module isn't yet used anywhere?)
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/story_tags.py (right): https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:8: class TagInfo(object): On 2017/02/03 16:35:29, perezju wrote: > On 2017/02/03 16:07:55, nednguyen wrote: > > On 2017/02/03 15:52:21, perezju wrote: > > > why not make this a collections.namedtuple? > > > > > > also, an opinion nit, I don't like much the distinction between "tags" and > > "tag > > > infos"; I could call this class "Tag" with a "name" property instead, the > > module > > > can then expose an ALL_TAGS list, while story classes define the TAGS they > > use. > > > > collections.namedtuple is mutable, so I prefer using @property to make it > > "immutable". Rename it to Tag sgtm > > No, it's not. It's a tuple subclass, and therefore not mutable. > > >>> import collections > >>> Tag = collections.namedtuple('Tag', ['name', 'description']) > >>> t = Tag('foo', 'bar') > >>> t.name = 'nope' > Traceback (most recent call last): > File "<stdin>", line 1, in <module> > AttributeError: can't set attribute > >>> t > Tag(name='foo', description='bar') > >>> Ah cool. Done. https://codereview.chromium.org/2651623005/diff/20001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/story_tags.py:61: assert obj.tag not in all_tags, 'Duplicate tag: %s' % obj.tag On 2017/02/03 16:35:29, perezju wrote: > On 2017/02/03 16:07:55, nednguyen wrote: > > On 2017/02/03 15:52:21, perezju wrote: > > > Maybe move this to a unit test? > > > > > > I think this would actually fail as it is now (because of the mistake of > > having > > > 'pinch-zoom' twice), and it would be good to catch this while running tests > > > rather than on live code. > > > > I think when there is inconsistent/wrong logic, it's better to fail hard > rather > > than having flawed output, especially for non end-users facing products like > > benchmarks. > > Yes, agree. What I was trying to say is that this should *also* fail on CQ. Not > sure why it passed all your try jobs above (maybe because the module isn't yet > used anywhere?) Oh, turn out that it was a bug in "for obj in dir(sys.modules[__name__]):" as it just return the global variable names After updating the code & deliberately make the mistake, the whole thing now fails because we invoke this as module level.
lgtm, thanks!
The CQ bit was unchecked by nednguyen@google.com
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rnephew@chromium.org Link to the patchset: https://codereview.chromium.org/2651623005/#ps60001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486142329272430,
"parent_rev": "0f2fe15362ba9aa674aedddbe243a0a6160cbad3", "commit_rev":
"6a67824709b57582fc3957f25b5abad81a90a324"}
Message was sent while issue was closed.
Description was changed from ========== [tools/perf] Create a list of tags for system_health_story BUG= ========== to ========== [tools/perf] Create a list of tags for system_health_story BUG= Review-Url: https://codereview.chromium.org/2651623005 Cr-Commit-Position: refs/heads/master@{#448026} Committed: https://chromium.googlesource.com/chromium/src/+/6a67824709b57582fc3957f25b5a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6a67824709b57582fc3957f25b5a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
