Issue 240070043: LayoutTreeSnapshot initializes LayoutTree snapshots. (Closed)
|
Created:
8 years, 11 months ago by benjhayden Modified:
8 years, 11 months ago CC:
trace-viewer-review_googlegroups.com Base URL:
git@github.com:google/trace-viewer@master Visibility:
Public. |
DescriptionLayoutTreeSnapshot initializes LayoutTree snapshots.
LayoutTreeSnapshot copies row.children to row.subRows for table builder. It will do more in future changes.
BUG=
Patch Set 1 #
Total comments: 34
Patch Set 2 : comments #Patch Set 3 : slowing down #Patch Set 4 : slowing down #Patch Set 5 : slowing down #
Total comments: 2
Patch Set 6 : comment #
Total comments: 2
MessagesTotal messages: 11
On 2015/05/21 23:39:09, benjhayden wrote: nit: The way to do descriptions with git is to have a single line at the top, less then 72 characters that is the summary. A blank line, and then the full description wrapped to ~72 characters below. This means you'll get nicer titles in git log and in github when looking at the history.
Sign in to reply to this message.
I find the tree construction really hard to puzzle through. Can you please create a quick document that shows, what comes out of chrome in the dump, how that gets built into a tree internally and how it will be displayed? I think it would make it a lot easier to figure out if these CLs are correct. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... File trace_viewer/extras/chrome/blink/layout_tree_snapshot.html (right): https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:26: ['positioned', 'relative', 'anonymous', 'float', 'row', 'col', 'rowSpan', Is this filtering necessary? How much does it really remove? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:28: root._has[col] = root._has[col] || !!row[col]; We don't typically prefix member names with _. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:30: root._has.needs = root._has.needs || row.selfNeeds || has isn't a good name. Has what, what does this map actually hold? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:34: function traverseTree(row, childrenPropertyName, cb) { Does childrenPropertyName really need to be generalized here? Do we have multiple trees inside each row? If not, lets just use subRows below to make this clearer. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:46: if (root._prev) { Why does finding the previous row also change the roots previous row? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:49: } If we alreayed have row._prev why do we need to find Prev, isn't it already set? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:50: if (row._prev) { What does row mean in this method? Row in the displayed table, row in the LayoutTree? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:52: row._prevMissing = false; Why is this flag needed. previous is missing if previous is undefined. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:54: row._prev._nextMissing = false; This method isn't really findPrev, it doesn't return anything and it sets up a bunch of pointers. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:56: // row is only missing from the previous snapshot if there was a previous What does it mean if row is missing from the previous snapshot? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:80: // If cycles such as row._prev._next are created during preInitialize(), Why would these cycles happen? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:90: if (snapshots[index].guid === this.guid) { if (snapshots[index].guid !== this.guid) next; https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:93: this.args._prev = this._prev.args; Why do we need this when we can just get it through the previous pointer?
Sign in to reply to this message.
> Can you please create a quick document that shows, what comes out of chrome in the dump, how that gets built into a tree internally and how it will be displayed? Sorry, I'm not sure what specifically is difficult to understand. Is layout_tree_test_data.js not a good enough example of the json that is generated by TracedLayoutObject? TracedLayoutObject in blink collects data from each LayoutObject (exactly one TracedLayoutObject instance per LayoutObject), and generates JSON when the trace is dumped. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Each TracedLayoutObject becomes a row in the LayoutTreeView's table builder; it's a 1-to-1 mapping. The json stores child nodes in the row.children array, but table builder iterates over row.subRows, so one of LayoutTreeSnapshot's primary responsibilities is copying row.children to row.subRows for each row. The screenshot of LayoutTreeView (with the patch that scrolls table builder's tbody) is still here: https://x20web.corp.google.com/~benjhayden/229690043.png https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... File trace_viewer/extras/chrome/blink/layout_tree_snapshot.html (right): https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:26: ['positioned', 'relative', 'anonymous', 'float', 'row', 'col', 'rowSpan', On 2015/05/22 14:27:49, dsinclair wrote: > Is this filtering necessary? How much does it really remove? There are currently 20 columns in LayoutTreeView. If all 20 are displayed, then the LayoutTreeView table would be wider than my 30" monitor, so there would be a horizontal scrollbar in the bottom panel. The horizontal scrollbar would be exceptionally annoying since I'm working on putting the vertical scrollbar *inside* the tbody so that the thead is always visible. Before a page is loaded, only about 5 columns are needed and the other 15 are filtered out. Once a complex page is fully loaded, usually about 15 columns are needed and the other 5 or so are filtered out, so the table fits in about 27"-28" of my 30" monitor. The filtering takes about a millisecond to compute, can save tens of ms out of 1.5 seconds to fully render a 2000-row table, and prevents an awkward horizontal scroll bar. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:28: root._has[col] = root._has[col] || !!row[col]; On 2015/05/22 14:27:49, dsinclair wrote: > We don't typically prefix member names with _. Done. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:30: root._has.needs = root._has.needs || row.selfNeeds || On 2015/05/22 14:27:49, dsinclair wrote: > has isn't a good name. Has what, what does this map actually hold? root._has.needs indicates that at least one row in this tree *has* the *needs* column. root._has.colSpan indicates that at least one row in this tree *has* the *colSpan* column. I'd thought that "hasFlags" would mislead readers to think that the tree has something called "flags". Renamed to hasColumns. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:34: function traverseTree(row, childrenPropertyName, cb) { On 2015/05/22 14:27:50, dsinclair wrote: > Does childrenPropertyName really need to be generalized here? Do we have > multiple trees inside each row? If not, lets just use subRows below to make this > clearer. Done. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:46: if (root._prev) { On 2015/05/22 14:27:50, dsinclair wrote: > Why does finding the previous row also change the roots previous row? Because the rowCache for the next snapshot has not been computed yet, so it would be expensive to find row._next while processing row, which might require an entire tree traversal, which would turn this O(N) algorithm into O(N*N). So we set row._prev._next while handling row, not while handling row._prev. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:49: } On 2015/05/22 14:27:50, dsinclair wrote: > If we alreayed have row._prev why do we need to find Prev, isn't it already set? Um, no, this is where we set row._prev. The condition is root._prev, not row._prev. So we only look for row._prev if there is a previous snapshot in root._prev. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:50: if (row._prev) { On 2015/05/22 14:27:50, dsinclair wrote: > What does row mean in this method? Row in the displayed table, row in the > LayoutTree? This file is LayoutTreeSnapshot. We haven't gotten to the displayed table (LayoutTreeView) yet, although there will be exactly one display table row per snapshot row, and the table builder handles the tr element for us, so none of the LayoutTree{Snapshot,View,ViewHelper} handles the tr element. This row corresponds to a single TracedLayoutObject, which also corresponds to exactly one display table row tr element, although none of the LayoutTree{Snapshot,View,ViewHelper} directly handles the tr element. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:52: row._prevMissing = false; On 2015/05/22 14:27:50, dsinclair wrote: > Why is this flag needed. previous is missing if previous is undefined. It seems unnecessary and visually noisy for the first snapshot to highlight every row green, or for the last snapshot to highlight every row red. So LayoutTreeViewHelper will only highlight a row green if there is a previous snapshot and if that row is missing from it; it will only highlight a row red if there is a next snapshot and if that row is missing from it. Currently, LayoutTreeViewHelper only has access to the current row, not the root, so LayoutTreeViewHelper can't look directly to see if there is a previous/next snapshot. If you like, I could add a pointer from every row back to the root so that LayoutTreeViewHelper can directly see if there is a previous/next snapshot. Settings these flags seemed clearer to me than setting row.root. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:54: row._prev._nextMissing = false; On 2015/05/22 14:27:50, dsinclair wrote: > This method isn't really findPrev, it doesn't return anything and it sets up a > bunch of pointers. What would you like its name to be? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:56: // row is only missing from the previous snapshot if there was a previous On 2015/05/22 14:27:50, dsinclair wrote: > What does it mean if row is missing from the previous snapshot? LayoutTreeViewHelper will highlight it green. If it is missing from the next snapshot, then LayoutTreeViewHelper will highlight it red. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:80: // If cycles such as row._prev._next are created during preInitialize(), On 2015/05/22 14:27:50, dsinclair wrote: > Why would these cycles happen? A row has a pointer to its previous snapshot and its next snapshot. So row._prev._next === row === row._prev._next._prev._next === row._prev._next._prev._next._prev._next === .... and iterObjectFieldsRecursively will try to iterate recursively over that infinite sequence, which will exceed the maximum recursion depth. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:90: if (snapshots[index].guid === this.guid) { On 2015/05/22 14:27:50, dsinclair wrote: > if (snapshots[index].guid !== this.guid) > next; Done. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:93: this.args._prev = this._prev.args; On 2015/05/22 14:27:50, dsinclair wrote: > Why do we need this when we can just get it through the previous pointer? LayoutTreeViewHelper requires each row to have its own _prev and _next pointers so that it doesn't have to go and find them, which would be an O(N*N) algorithm. this.args is the first row, so it, too, must have its own _prev and _next pointers.
Sign in to reply to this message.
https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... File trace_viewer/extras/chrome/blink/layout_tree_snapshot.html (right): https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:26: ['positioned', 'relative', 'anonymous', 'float', 'row', 'col', 'rowSpan', On 2015/05/22 18:08:25, benjhayden wrote: > On 2015/05/22 14:27:49, dsinclair wrote: > > Is this filtering necessary? How much does it really remove? > > There are currently 20 columns in LayoutTreeView. If all 20 are displayed, then > the LayoutTreeView table would be wider than my 30" monitor, so there would be a > horizontal scrollbar in the bottom panel. The horizontal scrollbar would be > exceptionally annoying since I'm working on putting the vertical scrollbar > *inside* the tbody so that the thead is always visible. > Before a page is loaded, only about 5 columns are needed and the other 15 are > filtered out. Once a complex page is fully loaded, usually about 15 columns are > needed and the other 5 or so are filtered out, so the table fits in about > 27"-28" of my 30" monitor. > > The filtering takes about a millisecond to compute, can save tens of ms out of > 1.5 seconds to fully render a 2000-row table, and prevents an awkward horizontal > scroll bar. So this only filters half the rows? This is also going to cause problems for people that don't have 30" monitors. Is columns the best way to show this information? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:49: } On 2015/05/22 18:08:25, benjhayden wrote: > On 2015/05/22 14:27:50, dsinclair wrote: > > If we alreayed have row._prev why do we need to find Prev, isn't it already > set? > > Um, no, this is where we set row._prev. > The condition is root._prev, not row._prev. > So we only look for row._prev if there is a previous snapshot in root._prev. The next line down is if (row.prev). So, we already have a previous row? If we have a previous row why are we in findPrev? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:50: if (row._prev) { On 2015/05/22 18:08:24, benjhayden wrote: > On 2015/05/22 14:27:50, dsinclair wrote: > > What does row mean in this method? Row in the displayed table, row in the > > LayoutTree? > > This file is LayoutTreeSnapshot. We haven't gotten to the displayed table > (LayoutTreeView) yet, although there will be exactly one display table row per > snapshot row, and the table builder handles the tr element for us, so none of > the LayoutTree{Snapshot,View,ViewHelper} handles the tr element. > This row corresponds to a single TracedLayoutObject, which also corresponds to > exactly one display table row tr element, although none of the > LayoutTree{Snapshot,View,ViewHelper} directly handles the tr element. If this is a layout object, can we call it that instead of row? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:52: row._prevMissing = false; On 2015/05/22 18:08:24, benjhayden wrote: > On 2015/05/22 14:27:50, dsinclair wrote: > > Why is this flag needed. previous is missing if previous is undefined. > > It seems unnecessary and visually noisy for the first snapshot to highlight > every row green, or for the last snapshot to highlight every row red. > So LayoutTreeViewHelper will only highlight a row green if there is a previous > snapshot and if that row is missing from it; it will only highlight a row red if > there is a next snapshot and if that row is missing from it. > Currently, LayoutTreeViewHelper only has access to the current row, not the > root, so LayoutTreeViewHelper can't look directly to see if there is a > previous/next snapshot. If you like, I could add a pointer from every row back > to the root so that LayoutTreeViewHelper can directly see if there is a > previous/next snapshot. Settings these flags seemed clearer to me than setting > row.root. Why does this need access to the root? You know if you have a previous pointer if row.previous is not undefined. If row.previous is undefined then it's missing. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:54: row._prev._nextMissing = false; On 2015/05/22 18:08:24, benjhayden wrote: > On 2015/05/22 14:27:50, dsinclair wrote: > > This method isn't really findPrev, it doesn't return anything and it sets up a > > bunch of pointers. > > What would you like its name to be? Something that describes what the method is actually doing. https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:56: // row is only missing from the previous snapshot if there was a previous On 2015/05/22 18:08:24, benjhayden wrote: > On 2015/05/22 14:27:50, dsinclair wrote: > > What does it mean if row is missing from the previous snapshot? > > LayoutTreeViewHelper will highlight it green. > If it is missing from the next snapshot, then LayoutTreeViewHelper will > highlight it red. Then this comment is badly phrased. I have no idea why the row would be missing if there is a previous snapshot, and the row may not be missing .... https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:80: // If cycles such as row._prev._next are created during preInitialize(), On 2015/05/22 18:08:24, benjhayden wrote: > On 2015/05/22 14:27:50, dsinclair wrote: > > Why would these cycles happen? > > A row has a pointer to its previous snapshot and its next snapshot. So > row._prev._next === row === row._prev._next._prev._next === > row._prev._next._prev._next._prev._next === .... and iterObjectFieldsRecursively > will try to iterate recursively over that infinite sequence, which will exceed > the maximum recursion depth. So, your iteration is walking down and back up the tree? Why does it do that? https://codereview.appspot.com/240070043/diff/1/trace_viewer/extras/chrome/bl... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:93: this.args._prev = this._prev.args; On 2015/05/22 18:08:24, benjhayden wrote: > On 2015/05/22 14:27:50, dsinclair wrote: > > Why do we need this when we can just get it through the previous pointer? > > LayoutTreeViewHelper requires each row to have its own _prev and _next pointers > so that it doesn't have to go and find them, which would be an O(N*N) algorithm. > this.args is the first row, so it, too, must have its own _prev and _next > pointers. Yes but why do you do this.args._prev = this._prev.args. You don't need to store that you can just get this._prev.args.
Sign in to reply to this message.
On 2015/05/22 18:08:25, benjhayden wrote: > > Can you please create a quick document that shows, what comes out of chrome in > the dump, how that gets built into a tree internally and how it will be > displayed? > > Sorry, I'm not sure what specifically is difficult to understand. > > Is layout_tree_test_data.js not a good enough example of the json that is > generated by TracedLayoutObject? > > TracedLayoutObject in blink collects data from each LayoutObject (exactly one > TracedLayoutObject instance per LayoutObject), and generates JSON when the trace > is dumped. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Each TracedLayoutObject becomes a row in the LayoutTreeView's table builder; > it's a 1-to-1 mapping. > > The json stores child nodes in the row.children array, but table builder > iterates over row.subRows, so one of LayoutTreeSnapshot's primary > responsibilities is copying row.children to row.subRows for each row. > > The screenshot of LayoutTreeView (with the patch that scrolls table builder's > tbody) is still here: > https://x20web.corp.google.com/~benjhayden/229690043.png > What is the tree structure your building. We have a LayoutTree in blink. We dump that out, each LayoutObject gets dumped. You read that in, build the LayoutTree, and then you hook each LayoutObject prev/next pointers up. Or something else? It seems like the model should model the layout tree and we work from there, no?
Sign in to reply to this message.
PTAL. Let's get the bare minimum in first and then take it one step at a time.
Sign in to reply to this message.
As I mentioned before, can you create a simple doc to show the input and the model you're planning on building. It's a lot easier to review if I can see where this is going. https://codereview.appspot.com/240070043/diff/80001/trace_viewer/extras/chrom... File trace_viewer/extras/chrome/blink/layout_tree_snapshot.html (right): https://codereview.appspot.com/240070043/diff/80001/trace_viewer/extras/chrom... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:45: row.subRows = row.children; Is this row not a layoutObject? Why row? Why duplicate children into subRows?
Sign in to reply to this message.
https://docs.google.com/document/d/19S7VQNve-0leCW-4JUunGS8OLBvv-93s-KXcIjDUC... https://codereview.appspot.com/240070043/diff/80001/trace_viewer/extras/chrom... File trace_viewer/extras/chrome/blink/layout_tree_snapshot.html (right): https://codereview.appspot.com/240070043/diff/80001/trace_viewer/extras/chrom... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:45: row.subRows = row.children; On 2015/05/22 19:57:26, dsinclair wrote: > Is this row not a layoutObject? Why row? Why duplicate children into subRows? Done.
Sign in to reply to this message.
https://codereview.appspot.com/240070043/diff/100001/trace_viewer/extras/chro... File trace_viewer/extras/chrome/blink/layout_tree_snapshot.html (right): https://codereview.appspot.com/240070043/diff/100001/trace_viewer/extras/chro... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:50: layoutObject.subRows = layoutObject.children; We shouldn't be setting our models up to work around the views. We should create the correct model and then make the views work. https://codereview.appspot.com/240070043/diff/100001/trace_viewer/extras/chro... trace_viewer/extras/chrome/blink/layout_tree_snapshot.html:56: if (!this.activeTree) { What is the active tree? Why does active tree have a different frame number then the index?
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
