Closed Bug 1238941 Opened 5 years ago Closed 5 years ago
Right pane memory table not updated when selecting snapshot
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
@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 ?
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.
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 ?)
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?
(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!
Caching was not previously tested, so no test update for removing it. Try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bb801686b2d
Comment on attachment 8708017 [details] [diff] [review] bug1238941.diff Review of attachment 8708017 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
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
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Try push up to this commit : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6ae046400eb
Commit 3cea1d886e9e did not trigger the intermittent tests and should be reapplied. (see Bug 1240368 for more info about the intermittent failures)
[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
You need to log in before you can comment on or make changes to this bug.