Closed Bug 1238941 Opened 8 years ago Closed 8 years ago

Right pane memory table not updated when selecting snapshot


(DevTools :: Memory, defect, P1)



(firefox46 fixed)

Firefox 46
Tracking Status
firefox46 --- fixed


(Reporter: jdescottes, Assigned: jdescottes)



(1 file, 1 obsolete file)

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.
Summary: Right pane tree table not updated when selecting snapshot → Right pane memory table not updated when selecting snapshot
Attached patch wip-patch.diff (obsolete) — Splinter Review
@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 ?
Flags: needinfo?(nfitzgerald)
Assignee: nobody → jdescottes
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.
Flags: needinfo?(nfitzgerald)
P1 because not having a broken tool is pretty fundamental.
Priority: -- → P1
@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 ?)
Flags: needinfo?(nfitzgerald)
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?
Flags: needinfo?(nfitzgerald)
(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!
Attached patch bug1238941.diffSplinter Review
Caching was not previously tested, so no test update for removing it.

Try push :
Attachment #8707425 - Attachment is obsolete: true
Attachment #8708017 - Flags: review?(nfitzgerald)
Comment on attachment 8708017 [details] [diff] [review]

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

Attachment #8708017 - Flags: review?(nfitzgerald) → review+
Thanks for the review, green on Linux and MacOS, no reason for this patch to be platform dependent.
-> adding checkin needed
Keywords: checkin-needed
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
I had to back this out for being a possible cause of frequent devtools test failures in browser_profiler_tree-abstract-02.js
Resolution: FIXED → ---
Try push up to this commit :
Flags: needinfo?(jdescottes)
Commit 3cea1d886e9e did not trigger the intermittent tests and should be reapplied. 

(see Bug 1240368 for more info about the intermittent failures)
Keywords: checkin-needed
Closed: 8 years ago8 years ago
Resolution: --- → FIXED


Comments: Test Successful
STR:  clear

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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.