Performance Tool struggles with large amount of markers (500 000)

RESOLVED FIXED in Firefox 52

Status

defect
P2
normal
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: fitzgen, Assigned: jsnajdr)

Tracking

(Blocks 3 bugs)

unspecified
Firefox 52
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

STR:

* e10s enabled (not sure if relevant or not)
* Load http://mozilla.pettay.fi/moztests/innerhtml_nonsimple_nostyle_test.html
* Start a recording
* Reload
* Stop recording

ER:

Loads profile in a timely manner.

AR:

Taking minutes long to load -- no end in sight. Beach balling and locking up whole browser.
I attached and recorded a profile with Instruments (the beach ball is still going, btw). This is a ten second recording.

Spending 99% of time in nsIPresShell::RemoveWeakFrameInternal???
Only hangs for about 5 or so seconds for me. The devtools profiler stop button doesn't do anything in a debug build for me, so I can't try to investigate whats happening in those seconds.
Probably much better after shu's changes land.
Priority: -- → P3
Can you see if this is still an issue? Should be much faster (< 1s even on large profiles, not counting rendering) with the deduping patch, and even with older profile formats, should be better.

Although the RemoveWeakFrameInternal time on that is very confusing.
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> Can you see if this is still an issue? Should be much faster (< 1s even on
> large profiles, not counting rendering) with the deduping patch, and even
> with older profile formats, should be better.
> 
> Although the RemoveWeakFrameInternal time on that is very confusing.
Flags: needinfo?(nfitzgerald)
I can still repro; just had to force quit firefox. I think this might have to do with markers rather than the SPS profiler data.
Flags: needinfo?(nfitzgerald)
I think it's rendering an obscene amount of markers in the waterfall view, but will investigate more
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Priority: P3 → P2
Yeah, I missed the beginning but recorded until the end and got 438872 markers.
The waterfall view is a bit faster now thanks to markers folding. That, combined with the fact that profiles are much more lightweight, should alleviate this but a little bit.
No longer blocks: perf-perf
The only remaining costly operation is bug 1169035.
Assignee: jsantell → nobody
Status: ASSIGNED → NEW
Summary: Super jank beach ball when loading a profile → Performance Tool struggles with large amount of markers (500 000)
This has a much more reasonable load with child markers nested. Regardless, it doesn't address the fact if we had 500,000 markers to render all at once (if there were no nesting), but that should be pretty unlikely/extreme.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #1)

> Spending 99% of time in nsIPresShell::RemoveWeakFrameInternal???

FWIW I am seeing this with a slightly different test case as well.
Looking there, I have 176105 nsWeakFrame objects in the linked list... whoops.
Bug 655084 claims to have fixed this but it seems not.
Blocks: 1159486
I discussed this with Jesup and he suggested I NI you.

See comment #12 for a bit of background.

I don't know anything about nsWeakFrame and the like.  So I have two questions.

One is, does this indicate some sort of bug in the performance tool front end?
Creating 171,105 nsWeakFrames seems a bit excessive perhaps, but I don't really know.
What are the typical reasons that these things are made?  Here I'm wondering how
I would go about tracking them down.

Second, bug 655084 suggested using a hash table rather than a linked list in nsIPresShell.
Is there any reason not to go ahead and do this?  I can give it a try and see whether it
fixes my problem.
Flags: needinfo?(roc)
Flags: needinfo?(ehsan)
I changed nsIPresShell to use a hash table for nsWeakFrames.  This made this
particular problem disappear -- when interrupting in gdb I no longer end up in
RemoveWeakFrameInternal.

However, the overall performance problem remains.  I didn't dig any deeper to
see what it is.  I did manage to find a way to reproduce it without my other patches
applied:

Run the browser.
Open http://mozilla.pettay.fi/moztests/events/event_speed_3.html
Open the performance tool
Start a recording
Click the "Click to start" button on the page

Now, if I stop the recording before ~3500ms, response is reasonable.
However, if I let it run too long, the browser appears to lock up.

Since this doesn't seem to be triggered by my patch after all,
I'm going to remove the dependency.
No longer blocks: 1159486
(In reply to Tom Tromey :tromey from comment #13)
> I discussed this with Jesup and he suggested I NI you.
> 
> See comment #12 for a bit of background.
> 
> I don't know anything about nsWeakFrame and the like.  So I have two
> questions.
> 
> One is, does this indicate some sort of bug in the performance tool front
> end?

I wouldn't say a bug per se, but this might be because of the specific things that the front-end is doing, yes.

> Creating 171,105 nsWeakFrames seems a bit excessive perhaps, but I don't
> really know.
> What are the typical reasons that these things are made?  Here I'm wondering
> how
> I would go about tracking them down.

We represent the "boxes" we use to render stuff into specific positions on the page as frames.  These are classes derived from nsIFrame.  These objects are transient, and they can be destroyed while parts of the page are alive.  For example, if you set something to display: none, the frames for that node and all of its descendants will get destroyed the next time that we process the dynamic changes.

nsWeakFrame objects are used to hold a weak reference to these frame objects in cases where we fear that the objects may get destroyed by the time we need to use them.

As to where these are coming from, presumbaly you should look for places where we store nsWeakFrames inside a linked list, and set a breakpoint where one such item is added and walk your way backwards to the place where we cause them to be created.

> Second, bug 655084 suggested using a hash table rather than a linked list in
> nsIPresShell.
> Is there any reason not to go ahead and do this?  I can give it a try and
> see whether it
> fixes my problem.

I'm not sure, I haven't looked at this stuff closely in quite some time.  roc or other layout folks should be able to help you.
Flags: needinfo?(ehsan)
(In reply to Tom Tromey :tromey from comment #14)
> Run the browser.
> Open http://mozilla.pettay.fi/moztests/events/event_speed_3.html
> Open the performance tool
> Start a recording
> Click the "Click to start" button on the page

I followed these steps and let the browser run for about 20s before clicking the stop button. I got the "Loading..." message and the progress icon spun for a bit and then locked up.

Attaching with gdb showed we were actually in the JS engine, and still in the JS engine even after I let it run for a while. I got this JS stack:
0 AbstractTreeItem( = [unavailable]) ["resource:///modules/devtools/AbstractTreeItem.jsm":122]
1 MarkerView( = [unavailable]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/performance/marker-view.js":40]
2 MarkerView.prototype<._populateSelf(children = [object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Object],[object Obje
Flags: needinfo?(roc)
Flags: needinfo?(vporof)
Flags: needinfo?(vporof)
Looking into this a bit more -- Chrome handles this by only having nodes for rows currently visible, and renders it impressingly well. Probably no other way to handle such a large amount of markers.
Duplicate of this bug: 1253561
The waterfall view now renders an unlimited number of markers, which hangs the browsers when there are thousands of them.

I'm going to rewrite this view to React, using the devtools/client/shared/components/tree component. It provides a virtualized view where only visible nodes are rendered, and updates the view on scroll.
QA Contact: jsnajdr
Assignee: nobody → jsnajdr
Blocks: perf-perf
QA Contact: jsnajdr
Duplicate of this bug: 1289070
Blocks: 1302060
No longer blocks: 1302060
Attachment #8790696 - Flags: review?(nfitzgerald)
I'm adding fitzgen to the review of the changes to the tree component.
Part 1 adds some useful improvements to the Tree component itself:

- add a "filter" property - a function that filters the nodes in the tree and renders only those who passed. Useful in the marker view to show only markers that are inside the desired time interval. Added a mochitest, too (test_tree_12.js)

- fixed bugs in computing the height of the bottom in the virtualized scroll view:

1. There was a off-by-one bug where the bottom spacer height was one itemHeight less than the right value. Fixed.
2. Never set the element's style.height property to negative value. This was happening when the tree height is smaller than the height of the available area. React checks the value and ignores it when it's negative. This leaves the actual CSS style of the element at the previous value. Has bad consequences when the tree gets filtered and therefore re-rendered from a long list to a short one. Amended test_tree_04.js to test the spacers.

- set "tree-node-first" and "tree-node-last" classes on the first and last rendered tree nodes. Makes styling the tree in the marker view easier.

- set "data-expanded" and "data-depth" attributes on the tree nodes. Again, helps a lot with styling.
Part 2 is a rewrite of the waterfall view to React. Several things worth explaining:

- components/marker-view.js contains the rendering code for both waterfall header and the tree. This code was previously (in non-React XUL form) in widgets/marker-view.js and widgets/waterfall-ticks.js
 
- unlike the XUL version, the "arrow" expander element is put together with the marker bar inside a "waterfall-marker-wrap" element, and doesn't therefore need to have its own "transform: translateX" property. This avoids having to do tricks with cloneElement and adding a new "style" prop to the ArrowExpander component. 

- in function collapseMarkersIntoNode in waterfall-utils.js, I add a numeric "index" property to every marker node. This serves as a unique "key" for the React lists in the Tree component. This "index" doesn't change when the marker list is transformed into a tree, when the markers are filtered by the time interval settings or hiddenMarker list, when the tree is expanded, collapsed or scrolled etc. Keeps DOM updates at a minimum.

- the widgets/waterfall-ticks.js doesn't contain the rendering code for the waterfall header any more. It now only contains utility functions for the "ticks" - drawing the ticks background and finding optimal tick interval.

- in views/details-waterfall.js, saving "lastSelected" property is no longer necessary - the React component takes care of it while updating itself.

- the styling in performance.css was tricky - the XUL flex layout and HTML flex don't cooperate very well, so I had to set "display: -moz-box" on several HTML elements I'd rather style with "display: flex". Otherwise, scrolling and other things wouldn't work correctly.

- the mochitests are updated to comply with changes in structure of markup and the view objects. No change in functionality.
Comment on attachment 8790696 [details]
Bug 1152441 - Part 1: Enhance the Tree component for use in performance waterfall

https://reviewboard.mozilla.org/r/78398/#review76988

We used to have filtering, but removed it in https://bugzilla.mozilla.org/show_bug.cgi?id=1236053

The reasons were

1. In the memory tool, we do filtering off-main-thread rather than on the main thread as we render, which is a superior architecture.
2. No one else was using the filtering at the time either.
3. We had no tests.

Reasons (2) and (3) have obviously changed: the performance tool wants to use filtering, and this new patch has tests.

However, I still believe very strongly that (1) is a superior architecture, and that the performance tool should move towards an architecture where the data lives off the main thread and sends the about-to-be-rendered subset of (pre-filtered, pre-massaged, etc) data to the main thread.

I realize that it is not reasonable to ask for the performance tool to be re-architected just to start using the tree component. At the same time, I don't want to encourage the inferior architecture.

With that in mind, I think making a new `FilteringTree` "middleware" component is the correct balance. This component would have a nice comment at the top warning people against doing data processing in the UI and on the main thread.

> In general, data processing (such as filtering) should not be performed while
> rendering. This will hurt FPS and responsiveness latency: it is extra work
> performed on the main thread, stealing precious time required for painting and
> layout, etc. This goes doubly for trees containing a large number of items,
> for which the `Tree` component is optimized for. Instead, the data should live
> on a worker, user filtering should be asynchronous, send a filtering request
> to the worker, which filters the data off main thread and sends it back to the
> main thread when it is ready for rendering. See how filtering is done in
> `devtools/client/memory` for an example.
>
> This `FilteringTree` component is an escape hatch: the above architecture is
> superior, but sometimes not practical for incremental changes. It will do
> filtering on the main thread for you, but its use with large trees should be
> avoided whenever possible.

This "middleware" component could be implemented in a straight forward manner with composition:

    const FilteringTree = createClass({
      displayName: "FilteringTree",
      render() {
        const { filter } = this.props;
        return Tree(Object.assign(this.props, {
          getRoots
        }))
      }
    });

::: devtools/client/shared/components/tree.js:583
(Diff revision 1)
>      ];
>  
>      for (let i = 0; i < toRender.length; i++) {
> +      let index = begin + i;
> +      let first = index == 0;
> +      let last = index == traversal.length - 1;

These bindings are not modified, so they should be `const` instead of `let`.
Attachment #8790696 - Flags: review?(nfitzgerald) → review-
Stupid mozreview lost a bunch of my review... Fixed `FilteringTree` below:

>     const FilteringTree = createClass({
>       displayName: "FilteringTree",
>       render() {
>         const { filter, getRoots, getChildren } = this.props;
>         return Tree(Object.assign(this.props, {
>           getRoots: () => getRoots().filter(filter),
>           getChildren: item => getChildren(item).filter(filter),
>         }))
>       }
>     });
Comment on attachment 8790697 [details]
Bug 1152441 - Part 2: Rewrite the marker view to use React and the Tree component

https://reviewboard.mozilla.org/r/78400/#review77078

::: devtools/client/performance/components/marker-view.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I think these components should be renamed and broken down into individual files. I've been following the style of the memory tool in the React refactor. In the memory tool each component is its own file, which is also my preference on organization. You can see the balance struck on how much to include on a file with the `census*.js` and `dominator-tree*.js` files:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/components

I included a few suggestions down below on organizational changes.

I also think the "marker" terminology is a throwback to the original code. I would prefer to have the code match the labels in the tool. I think using Waterfall rather than Marker would make sense.

::: devtools/client/performance/components/marker-view.js:27
(Diff revision 1)
> +const WATERFALL_HEADER_TEXT_PADDING = 3;
> +
> +/**
> + * The "waterfall ticks" view, a header for the markers displayed in the waterfall.
> + */
> +const MarkerHeader = createFactory(function (props) {

Rename to: `WaterfallHeader`
Move to: `waterfall-header.js`

Is there any reason that this isn't a createClass? I would prefer them all to use the typical pattern for creating components. Plus I think it would be good to have the displayName property on all of these.

If the components are broken out to different files then the pattern `const WaterfallHeader = createFactory(require("devtools/client/performance/components/waterfall-header"));` can be used, which is more typical across the tools.

::: devtools/client/performance/components/marker-view.js:28
(Diff revision 1)
> +
> +/**
> + * The "waterfall ticks" view, a header for the markers displayed in the waterfall.
> + */
> +const MarkerHeader = createFactory(function (props) {
> +  let { startTime, dataScale, sidebarWidth, waterfallWidth } = props;

These components' props are fairly well defined and stable. I think we should go ahead and add the React propTypes to them all. This could be a follow up bug if you want.

::: devtools/client/performance/components/marker-view.js:74
(Diff revision 1)
> +// px
> +const WATERFALL_MARKER_TIMEBAR_WIDTH_MIN = 5;
> +// px - keep in sync with var(--waterfall-tree-row-height) in performance.css
> +const WATERFALL_TREE_ROW_HEIGHT = 15;
> +
> +const MarkerRow = createFactory(createClass({

Rename to: `WaterfallRow`
Move to: `waterfall-row.js`

::: devtools/client/performance/components/marker-view.js:153
(Diff revision 1)
> +      timebarCell
> +    );
> +  }
> +}));
> +
> +const MarkerTree = createFactory(createClass({

Rename to: `WaterfallTree`
Move to: `waterfall-tree.js`

::: devtools/client/performance/components/marker-view.js:277
(Diff revision 1)
> +      itemHeight: WATERFALL_TREE_ROW_HEIGHT
> +    });
> +  }
> +}));
> +
> +const MarkerView = createFactory(function (props) {

Rename to: `Waterfall`
Move to: `waterfall.js`

Although this one will be almost empty... hmm...

::: devtools/client/performance/views/details-waterfall.js:85
(Diff revision 1)
>      this.details.off("resize", this._onResize);
>      this.details.off("view-source", this._onViewSource);
>      this.details.off("show-allocations", this._onShowAllocations);
>      window.removeEventListener("resize", this._onResize);
> +
> +    ReactDOM.unmountComponentAtNode(this.markersContainer);

Huh, I had no idea this was even a thing.

::: devtools/client/performance/views/details-waterfall.js:223
(Diff revision 1)
> +  _recalculateBounds: function () {
> +    let doc = this.markersContainer.ownerDocument;
> +    let win = doc.defaultView;
> +    let utils = win.QueryInterface(Ci.nsIInterfaceRequestor)
> +                   .getInterface(Ci.nsIDOMWindowUtils);
> +    let bounds = utils.getBoundsWithoutFlushing(this.markersContainer);

I don't think we should add more privileged code at this point that will need to be factored out for the devtools.html project.

::: devtools/client/performance/views/details-waterfall.js:237
(Diff revision 1)
> -      // The waterfall tree should not expand by default.
> -      autoExpandDepth: 0
> -    });
>  
> -    let header = new WaterfallHeader(root);
> +    let doc = this.markersContainer.ownerDocument;
> +    let startTime = interval.startTime|0;

nit: Are there other instances of using bitwise operators to set default values in the devtools code? It seems like `let startTime = interval.starTime || 0` is a more common practice and might be clearer to ensure a numeric value here. Plus I think there needs to be a space between operators.

::: devtools/client/performance/views/details-waterfall.js:243
(Diff revision 1)
> +    let dataScale = this.waterfallWidth / (endTime - startTime);
>  
> -    this._markersRoot = root;
> +    this.canvas = TickUtils.drawWaterfallBackground(doc, dataScale, this.waterfallWidth);
> -    this._waterfallHeader = header;
>  
> -    root.filter = this._hiddenMarkers;
> +    let markerView = MarkerView({

Thanks, this looks like a nice step towards an eventual redux worflow.

::: devtools/client/themes/performance.css:405
(Diff revision 1)
>  /**
>   * Waterfall ticks header
>   */
>  
> +#waterfall-markers {
> +  display: -moz-box;

I'd really prefer to use standards-compliant properties. If this is a hack to get it to work in a XUL context, then can you add a DE-XUL comment like I've done [here](https://dxr.mozilla.org/mozilla-central/rev/b1156b0eb96fcb49966b20e5fcf6a01f634ea2ee/devtools/client/themes/performance.css#31-37) so that we know to go back and remove it. Otherwise I would prefer to use non-prefixed properties.

::: devtools/client/themes/performance.css:457
(Diff revision 1)
>   * Markers waterfall breakdown
>   */
>  
> -#waterfall-breakdown {
> +.waterfall-markers .tree {
> +  display: -moz-box;
> +  -moz-box-orient: vertical;

See the DE-XUL comment above.

::: devtools/client/themes/performance.css:602
(Diff revision 1)
>  
>  /**
>   * OTMT markers
>   */
>  
> -.waterfall-tree-item[otmt=true] .waterfall-marker-bullet,
> +.waterfall-tree-item[data-otmt=true] .waterfall-marker-bullet,

Nice catch.
Comment on attachment 8790697 [details]
Bug 1152441 - Part 2: Rewrite the marker view to use React and the Tree component

https://reviewboard.mozilla.org/r/78400/#review77122

Sweet, nice job on this. Overall looks great. I have a few suggestions on some of the code organization and naming, plus a few issues here and there I noticed. I'll take myself off of the review for the tree component since fitzgen handled all of that. Thanks for doing this!
Attachment #8790697 - Flags: review?(gtatum) → review-
Attachment #8790696 - Flags: review?(gtatum)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #27)
> With that in mind, I think making a new `FilteringTree` "middleware"
> component is the correct balance. This component would have a nice comment
> at the top warning people against doing data processing in the UI and on the
> main thread.

I removed the filtering code (and the test) from the Tree component. The filtering logic is now directly embedded in the getRoots and getChildren implementation in waterfall-tree.js.

I also changed the 'let' bindings to 'const'.
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #29)
> I think these components should be renamed and broken down into individual
> files. I've been following the style of the memory tool in the React
> refactor.

Done, the components are divided into 4 files:
- waterfall-header.js
- waterfall-tree-row.js
- waterfall-tree.js
- waterfall.js (header + tree)

> I also think the "marker" terminology is a throwback to the original code.

Converted to "waterfall".

> Is there any reason that this isn't a createClass? I would prefer them all
> to use the typical pattern for creating components. Plus I think it would be
> good to have the displayName property on all of these.

As these components only have the "render" method and have no state, only props, I'm using stateless functional components. Described here:

https://medium.com/@joshblack/stateless-components-in-react-0-14-f9798f8b992d

The new console frontend already makes heavy use of them:

http://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/components/message-icon.js

The stateless components can be exported from modules and used with createFactory and createElement, just like any other component.

I added displayName to all stateless components I created.

> These components' props are fairly well defined and stable. I think we
> should go ahead and add the React propTypes to them all. This could be a
> follow up bug if you want.

Added propTypes to all waterfall components.

> > +    ReactDOM.unmountComponentAtNode(this.markersContainer);
> 
> Huh, I had no idea this was even a thing.

Oh, you absolutely need to call this when you're about to destroy the element where the React component got mounted. Otherwise, React will keep internal references to the elements forever. Leaks!

> > +    let utils = win.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                   .getInterface(Ci.nsIDOMWindowUtils);
> > +    let bounds = utils.getBoundsWithoutFlushing(this.markersContainer);
> 
> I don't think we should add more privileged code at this point that will
> need to be factored out for the devtools.html project.

This is moved from the original code. It does the same thing as this.markersContainer.clientWidth, but in an optimized way, without triggering layout. I converted this to the clientWidth call, so that we don't need to remove the privileged code later.

> > +#waterfall-markers {
> > +  display: -moz-box;
> 
> I'd really prefer to use standards-compliant properties. If this is a hack
> to get it to work in a XUL context, then can you add a DE-XUL comment

I'd love to get rid of the XUL -moz-box style, too, but I wasn't able to get the layout to work properly (mainly scrolling) with regular flex. These styles can be fixed immediately after performance.xul stops being XUL and is converted to HTML. Added the DE-XUL comments for now.
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #29)
> > +    let startTime = interval.startTime|0;
> 
> nit: Are there other instances of using bitwise operators to set default
> values in the devtools code? It seems like `let startTime =
> interval.starTime || 0` is a more common practice and might be clearer to
> ensure a numeric value here. Plus I think there needs to be a space between
> operators.

I copied these "|0" things from the original code, from places like this: 

http://searchfox.org/mozilla-central/source/devtools/client/performance/modules/widgets/marker-view.js#99-100

The "|0" actually doesn't provide a default value (the startTime and endTime always have reasonable values provided by the caller), but it's a neat asm.js trick to coerce JS numbers (56-bit floats) to 32-bit integers:

https://en.wikipedia.org/wiki/Asm.js#Code_generation
https://youtu.be/9OHcJzJQ2Nk?t=13m25s

I have no idea though why it's being used here. Victor is the original author, setting ni?
Flags: needinfo?(vporof)
Comment on attachment 8790697 [details]
Bug 1152441 - Part 2: Rewrite the marker view to use React and the Tree component

https://reviewboard.mozilla.org/r/78400/#review77264

Code looks great! Like I said on IRC, this really makes the tool more performant. R+, but I did notice a few small regressions that should be fixed before merging, plus fitzgen's changes.

Regression: Waterfall view loses focus on using a keyboard shortcut.

STR:
 * Click on a waterfall item.
 * On mac hit fn + down.
 * The screen jumps down.
 * Hit fn + down.
 * The waterfall view has lost focus and the shortcut doesn't work.

Expected behavior:
 * The waterfall view retains focus while hitting shortcuts.

I had a similar issue due to XUL focus bugs. If I recall correctly I had to manually do a combo of window.focus() and element.focus() to get this to work correctly. It may have to do with hiding elements or removing elements in XUL that have the focus. I would also add the DE-XUL comment to the code as I believe this will be a hack to get it to work in XUL.

Regression: Gap at the bottom of the view.

I suspect that your off by one issue in the previous tree code commit was actually needed. There is a slight gap at the bottom of the view that could be filled in with 1 more item that would then be partially obscured. This is the observed behavior in Aurora.
Attachment #8790697 - Flags: review?(gtatum) → review+
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #36)
> Regression: Waterfall view loses focus on using a keyboard shortcut.

I added your hack from bug 1259228 to the tree component and ... it works! When a focused tree node is being removed, I check for that in componentWillUnmount(), blur the node-to-be-removed and manually focus the tree element. I tested if this still works if the whole tree is being unmounted - there is a danger of touching a ".tree" element after being unmounted - and it seems to not break anything.

> Regression: Gap at the bottom of the view.
> 
> I suspect that your off by one issue in the previous tree code commit was
> actually needed. There is a slight gap at the bottom of the view that could
> be filled in with 1 more item that would then be partially obscured. This is
> the observed behavior in Aurora.

This issue already existed in the tree component - I can reproduce it in the memory tool in Nightly/Aurora. The begin/end indexes were incorrectly computed when rendering the tree.

Say that the item height is 10px, and the tree view is 12px high. This means that, when scrolled to the right position, the view can display 3 items at once. However, the algorithm that computes the begin/end values at [1] will display only the first two items, even after NUMBER_OF_OFFSCREEN_ITEMS is added.

Fixed and amended the test_tree_04.js test.

[1] http://searchfox.org/mozilla-central/source/devtools/client/shared/components/tree.js#556-559
Comment on attachment 8790696 [details]
Bug 1152441 - Part 1: Enhance the Tree component for use in performance waterfall

https://reviewboard.mozilla.org/r/78398/#review78182

LGTM
Attachment #8790696 - Flags: review?(nfitzgerald) → review+
Blocks: 1169144
I had to fix one more thing: when doing the focus hack described in comment 41, I need to check if this.refs.tree is null - when unmounting the Tree component, the refs are reset.

This problem caused failures on try in devtools/client/memory tests. After fixing, the latest try run has green dt* tests.
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9de1722d996d
Part 1: Enhance the Tree component for use in performance waterfall r=fitzgen
https://hg.mozilla.org/integration/autoland/rev/7d2da44fd218
Part 2: Rewrite the marker view to use React and the Tree component r=gregtatum
https://hg.mozilla.org/mozilla-central/rev/9de1722d996d
https://hg.mozilla.org/mozilla-central/rev/7d2da44fd218
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Duplicate of this bug: 1310742
Product: Firefox → DevTools
Flags: needinfo?(vporof)
You need to log in before you can comment on or make changes to this bug.