Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
302443006:
binary_size: Avoid creating nodes with many thousand children. (Closed)
|
Created:
6 years, 7 months ago by Daniel Bratell Modified:
6 years, 6 months ago CC:
chromium-reviews, Primiano Tucci (use gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionbinary_size: Avoid creating nodes with many thousand children.
If a bucket has too many thousand children the webapp (graph lib?)
hangs. Protect against the most common case by splitting up the
(No Path) section in chunks of 3000 symbols.
BUG=370377
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276650
Patch Set 1 #
Total comments: 19
Patch Set 2 : (No Path): Addressed review comments #Patch Set 3 : Split (no path) Rebased to newer master #Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:67: NODE_TYPE_KEY = 'k' Globals and constatns should go between imports and the first top level class/function (i.e. on line 27). Remember two newlines between top levels (i.e. between import and globals, and one between globals and the first def) https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:76: child = node[NODE_CHILDREN_KEY].get(name) To be honest, sounds like your nodes should start being typed objects at this point, something like class Node(object): def __init__(self, ...): self.name = ... self.children = ... instead of keep defining constants to populate a dictionary with well known keys. That's why python objects are essentially dictionaries, to avoid you hacking dictionaries yourself. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:84: BIG_BUCKET_LIMIT = 2000 Ditto. Globals should live all together happily in the initial part of the module. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:114: no_path_bucket[NODE_CHILDREN_KEY] = new_children Uh? Looks like you already did this on line 99? Or am I missing something? https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:133: """Puts symbol into the file path node |node|. Returns the number of Nit: the first line of a docstring should fit in one line. Eventually, the rest can go after one blank line, i.e.: """Puts symbol into the file path node |node|. Returns the number of added levels in tree. I.e. returns 2.""" Also, from a purely latin roots standpoint, that should be "e.g.," not "i.e."... unless all your trees in this module are of height two, in which case this function should just "return 2" :) https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:191: node[NODE_TYPE_KEY] = 'p' # p for path Nit: add an extra space before #
I'm somewhat conflicted on this change. We have a general problem to solve for *all* buckets, not just no-path. Munging the data feels like a hack to me; I'd rather keep the data structure "pure" and deal with this in the layout logic within the UI. That said I don't think this is a major change, and when I have time do the UI work we could revisit this. Would you be ok with burning the big-bucket logic after the UI improves a bit? Again, thanks for the improvements to the tool! https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:84: BIG_BUCKET_LIMIT = 2000 Minor nit, the review says 3k and this constant is initialized to 2k. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:91: if '(No Path)' in root_children: We should probably extract the string "(No Path)" into a constant at this point, or keep a special reference to it around. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:136: depth = 0 From a clarity standpoint I'd prefer if we avoid the raw word "depth" here, since there's already an absolute notion of depth in each node. How about "levels_added" or "deepend_by", etc? https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:138: # 'node' is now the file node. Find the symbol-type bucket. The first part of this comment made sense in-situ, but now that you've transplanted it you could probably ditch this comment.
Yes, this should go away as soon as possible. If I had easily been able to fix the graphing code I would have done that instead from the start. There were actually two problems I ran into: (old) Firefox has a limit on the number of items in a json array, think it's fixed, and something that looked like a pure hang (performance I assume). No browser behaved well so I suspect it's something in the script. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:67: NODE_TYPE_KEY = 'k' On 2014/05/27 09:24:17, Primiano Tucci wrote: > Globals and constatns should go between imports and the first top level > class/function (i.e. on line 27). Remember two newlines between top levels (i.e. > between import and globals, and one between globals and the first def) Done. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:76: child = node[NODE_CHILDREN_KEY].get(name) On 2014/05/27 09:24:17, Primiano Tucci wrote: > To be honest, sounds like your nodes should start being typed objects at this > point, something like > > class Node(object): > def __init__(self, ...): > self.name = ... > self.children = ... > > instead of keep defining constants to populate a dictionary with well known > keys. That's why python objects are essentially dictionaries, to avoid you > hacking dictionaries yourself. I agree, but I'd prefer to postpone that change to a later cleanup pass. It would require some custom serialization code since the structures need to look as dicts when serialized through json so it is big enough to warrant a pass of its own. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:84: BIG_BUCKET_LIMIT = 2000 On 2014/05/27 09:24:17, Primiano Tucci wrote: > Ditto. Globals should live all together happily in the initial part of the > module. Done. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:84: BIG_BUCKET_LIMIT = 2000 On 2014/05/27 10:04:11, Andrew Hayden wrote: > Minor nit, the review says 3k and this constant is initialized to 2k. Done. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:91: if '(No Path)' in root_children: On 2014/05/27 10:04:11, Andrew Hayden wrote: > We should probably extract the string "(No Path)" into a constant at this point, > or keep a special reference to it around. Done. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:114: no_path_bucket[NODE_CHILDREN_KEY] = new_children On 2014/05/27 09:24:17, Primiano Tucci wrote: > Uh? Looks like you already did this on line 99? Or am I missing something? Yes, duplicate assignments. Fixed. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:133: """Puts symbol into the file path node |node|. Returns the number of On 2014/05/27 09:24:17, Primiano Tucci wrote: > Nit: the first line of a docstring should fit in one line. Eventually, the rest > can go after one blank line, i.e.: > > """Puts symbol into the file path node |node|. > > Returns the number of added levels in tree. I.e. returns 2.""" > > Also, from a purely latin roots standpoint, that should be "e.g.," not "i.e."... > unless all your trees in this module are of height two, in which case this > function should just "return 2" :) They are always of height 2. :-) This is extracted code. I kept the code that became more or less meaningless as a documentation to the reader. Changed it to "return 2" with a comment. As you may have noticed I like doing multiple pass over code when refactoring it. If every pass does a relatively small change it becomes easier to review and harder to introduce bugs. It also makes the refactorings focus in on the areas where they have the most values. Otherwise my experience is that refactorings can become depth-first and you end up cleaning up code nobody ever reads and that worked perfectly well. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:136: depth = 0 On 2014/05/27 10:04:11, Andrew Hayden wrote: > From a clarity standpoint I'd prefer if we avoid the raw word "depth" here, > since there's already an absolute notion of depth in each node. How about > "levels_added" or "deepend_by", etc? Done. https://codereview.chromium.org/302443006/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:138: # 'node' is now the file node. Find the symbol-type bucket. On 2014/05/27 10:04:11, Andrew Hayden wrote: > The first part of this comment made sense in-situ, but now that you've > transplanted it you could probably ditch this comment. Done.
Can you please take a look at the new patch and the comments.
LGTM!
The CQ bit was checked by bratell@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/302443006/10001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by andrewhayden@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/302443006/10001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by andrewhayden@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/302443006/10001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by bratell@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/302443006/30001
Message was sent while issue was closed.
Change committed as 276650 |
