1238941 - Right pane memory table not updated when selecting snapshot

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P1)

Description

[Julian Descottes [:jdescottes]]

STRs : 
- go to any page
- open memory tool
- take two memory snapshots
- expand the "objects" treenode
- select the first snapshot

Actual : "objects" node is still expanded
Expected : "objects" node is collapsed

This is not just about the state of the tree table, the content of the tree table does not match the selected snapshot.

As soon as you toggle one of the tree nodes, the tree table will then be updated with the correct content.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: wip-patch.diff

@fitzgen : Looks like the issue is linked to the DFS caching added in Bug 1219071.

When we select another snapshot, the instance of Tree component remains the same and this.state.reuseCachedTraversal is coming from the previous state of the Tree.

I think we should find a way to invalidate the cache when the roots are changing. I added a new mochitest to illustrate the issue, as well as a temporary "fix" (keeping a reference on the roots when we create the cache). But I'm pretty sure I got the wrong approach here.

Any idea what would be a cleaner fix for this ?

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Interesting. I had assumed that the component was re-created when switching between views, but when I think about it from React's perspective rather than my subjective perspective based on my intuition of the semantics of what we are rendering, I understand why it is not re-created.

We can either back out the traversal caching (I think it was not as effective as rate limiting various events to once per animation frame that we did later) or we can patch the should-we-reuse-the-cached-traversal? logic to consider the root node of the census. I would prefer the former given that it is a simplification of the code, but iff we can verify that the DFS isn't really taking that much time in the render() method and that removing the caching doesn't slow down performance of the tree scrolling.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

P1 because not having a broken tool is pretty fundamental.

Comment 4

[Julian Descottes [:jdescottes]]

@Nick : Looks like the caching was already removed for the census view in Bug 1229229. Not sure this was intentional but I think it's for the best. We can use _this_ bug to figure out if we enable caching again and how.

The memory table without caching is "usable" but feels more laggy. I did not run thorough benchmarks, but the difference is noticeable on my machine (MBP 17", i7 2.8 GHz). Most importantly, when scrolling the user sees a lot more "blank space".

I would be in favor of keeping the caching mechanism. I think the tree component is the best candidate to know when to invalidate the cache. The cache is based on the tree roots, so if the tree roots change, we should invalidate the cache.

Some options :
- keep a reference to the roots (as I did in Attachment 8707425 [details] [diff])
- use the root ids to create a hash we can check
- store the cache in the roots themselves (don't know a lot about react, but I feel we shouldn't modify objects here though ?)

Comment 5

[Julian Descottes [:jdescottes]]

Ignore my last comment.

I did a new batch of tests, actually profiling the time spend in JS while scrolling. In a react "runBatchUpdate" of ~35ms, I spend ~2ms in _dfsFromRoots, so around 6%.

And running a few more tests, the difference between with and without cache doesn't seem that huge anymore.

With that in mind, I think it's not worth keeping the cache. Should we use this bug to remove the cache implementation from the tree component?

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Julian Descottes from comment #5)
> With that in mind, I think it's not worth keeping the cache. Should we use
> this bug to remove the cache implementation from the tree component?

Sounds good to me! Thanks for measuring!

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: bug1238941.diff

Caching was not previously tested, so no test update for removing it.

Try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bb801686b2d

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8708017 [details] [diff] [review]
bug1238941.diff

Review of attachment 8708017 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 9

[Julian Descottes [:jdescottes]]

Thanks for the review, green on Linux and MacOS, no reason for this patch to be platform dependent.
-> adding checkin needed

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3cea1d886e9e

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3cea1d886e9e

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for being a possible cause of frequent devtools test failures in browser_profiler_tree-abstract-02.js

https://treeherder.mozilla.org/logviewer.html#?job_id=6636210&repo=fx-team
https://hg.mozilla.org/mozilla-central/rev/03506c7d53c5

Comment 13

[Julian Descottes [:jdescottes]]

Try push up to this commit : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6ae046400eb

Comment 14

[Julian Descottes [:jdescottes]]

Commit 3cea1d886e9e did not trigger the intermittent tests and should be reapplied. 

(see Bug 1240368 for more info about the intermittent failures)

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b857500874d8

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b857500874d8

Comment 17

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments: Test Successful
STR:  clear


Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Object node had collapsed already. After clicking, it expanded.

Actual Results: 
As expected