Closed
Bug 1399265
Opened 7 years ago
Closed 7 years ago
Switching from "Raw Data" to "JSON" tab hangs
Categories
(DevTools :: JSON Viewer, defect)
Tracking
(Performance Impact:?, firefox-esr52 unaffected, firefox56 wontfix, firefox57 fixed, firefox58 fixed)
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox56 | --- | wontfix |
firefox57 | --- | fixed |
firefox58 | --- | fixed |
People
(Reporter: frank, Assigned: Oriol)
References
(Depends on 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Honza
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
This can be easily verified in about:telemetry, viewing the Raw JSON data. First go to the "Raw Data" tab, then switch back to the "JSON" tab - it hangs for >40s for me. Here is the profile: https://perfht.ml/2wnZH6p Note: I've tested this on smaller JSON blobs, and it is definitely still present, though doesn't take quite as long (try Histograms.json, for example).
Updated•7 years ago
|
Product: Core → Toolkit
Whiteboard: [qf]
Comment 1•7 years ago
|
||
Sean, can you please take a quick look at the profile? Thanks!
Flags: needinfo?(sstangl)
Comment 2•7 years ago
|
||
The attached profile unfortunately doesn't have enough information, because the buffer was too small. I was able to reproduce the lag locally with a much larger buffer: https://perfht.ml/2jDm2vw That profile shows an 11,374ms pause in nsLineBox::RFindLineContaining(). It seems like we're hitting a pathological case during reflow. Olli, do you see anything meaningful in the callstack of that function?
Flags: needinfo?(bugs)
Comment 3•7 years ago
|
||
I'm not too familiar with reflow. -> dholbert
Flags: needinfo?(bugs) → needinfo?(dholbert)
Comment 4•7 years ago
|
||
So, it seems this is the JSON viewer doing something silly, which happens to trigger a slow case in Gecko. In the STR, when you switch between views, the way our JSON viewer hides the current view is: it restyles it to have "height:0; width:0; visibility:hidden". And so, Gecko dutifully tries to layout the content into that width. Unfortunately, in this case, we have megabytes of text, all on a single line, with "white-space: pre-wrap". And as it happens, that's one of the things that triggers exponential layout of some sort, which I think is tracked in bug 643918. I don't know enough about the linebreaking code in play here to know whether we can optimize it to avoid this blowup. (Maybe "width:0" might even be an edge case that we could add some special-case handling for?) But I suspect the most direct way to avoid this would be for the JSON viewer to hide this content by styling it with "display:none", so that we actually get rid of it rather than trying to lay it out into a width:0 container.
Flags: needinfo?(dholbert)
Comment 5•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #4) > But I suspect the most direct way to avoid this would be for the JSON viewer > to hide this content by styling it with "display:none", so that we actually > get rid of it rather than trying to lay it out into a width:0 container. Moving to Toolkit::Telemetry in the hope that people who work on about:telemetry can look into this.
status-firefox57:
--- → fix-optional
Component: General → Telemetry
Comment 6•7 years ago
|
||
Nothing to do with us, I'm afraid. Passing to JSON viewer component: Anyone here have a cycle to spare to turn a width:0 height:0 into a display: none? Or half a cycle to point out the offending code?
Component: Telemetry → Developer Tools: JSON Viewer
Product: Toolkit → Firefox
Comment 7•7 years ago
|
||
(In reply to Chris H-C :chutten from comment #6) > Passing to JSON viewer component: Anyone here have a cycle to spare to turn > a width:0 height:0 into a display: none? Actually, the "width: 0" is really the problem here -- that's what triggers the slow relayout of the resized pane's contents. And I *think* the page renders the same (but faster) without that -- the remaining "height:0;visibility:none" styles should probably be sufficient to hide the pane. > Or half a cycle to point out the > offending code? I don't actually know what JS is responsible for making these tweaks -- I'm just using the DOM inspector and am observing it happening (the style attribute being manipulated). As a "proof of concept", here's one extremely hacky solution: - Edit this file: https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/css/json-panel.css ...to add the following rule: .hidden { width: 100% !important; } This overrides the problematic "width:0" that some JS sticks into the style attribute when hiding this pane. This is probably not actually how we'd like to solve the problem, but it's maybe a good way to prototype whether that's sufficient. This also works but is more overkill since it triggers frame destruction/reconstruction whenever we hide/show a pane: .hidden { display: none; }
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8911556 [details] Bug 1399265 - Avoid relayout when switching tab in devtools https://reviewboard.mozilla.org/r/182988/#review188162 ::: devtools/client/shared/components/tabs/tabs.js:334 (Diff revision 1) > - // than display:none and visibility:collapse. > + // destruction and reconstruction. 'width' is not changed to avoid relayout. > let style = { > visibility: selected ? "visible" : "hidden", > height: selected ? "100%" : "0", > - width: selected ? "100%" : "0", > }; The styles could also be moved to the stylesheet: ```css .tab-panel-box.hidden { visibility: hidden; height: 0; } ``` But maybe they are here to increase their specificity.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8911556 [details] Bug 1399265 - Avoid relayout when switching tab in devtools https://reviewboard.mozilla.org/r/182990/#review188246 Thanks for tracking down the code to fix! This makes sense to me conceptually -- but I don't know this code at all, so I probably shouldn't r+ any changes here. Could you request review from someone who has touched this file before? Maybe Jan Odvarko (:Honza) who has hg blame on this chunk of code that you're modifying?
Attachment #8911556 -
Flags: review?(dholbert)
Updated•7 years ago
|
Flags: needinfo?(sstangl) → needinfo?(oriol-bugzilla)
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8911556 [details] Bug 1399265 - Avoid relayout when switching tab in devtools https://reviewboard.mozilla.org/r/182990/#review188384
Assignee | ||
Updated•7 years ago
|
Attachment #8911556 -
Flags: review?(odvarko)
Assignee | ||
Comment 12•7 years ago
|
||
Since it is a relayout problem I thought you would be more appropriate. Switched review to Honza.
Flags: needinfo?(oriol-bugzilla)
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8911556 [details] Bug 1399265 - Avoid relayout when switching tab in devtools https://reviewboard.mozilla.org/r/182990/#review188768 Looks great to me, thanks Oriol! Honza
Attachment #8911556 -
Flags: review?(odvarko) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 14•7 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/94a5ac215768 Avoid relayout when switching tab in devtools r=Honza
Keywords: checkin-needed
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/94a5ac215768
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Assignee | ||
Comment 16•7 years ago
|
||
Is this worth uplifting to firefox57?
Blocks: 1248380
status-firefox55:
--- → unaffected
status-firefox56:
--- → wontfix
status-firefox-esr52:
--- → unaffected
Keywords: regression
Version: unspecified → 56 Branch
Reporter | ||
Comment 17•7 years ago
|
||
*Way* faster now, thanks for the fix!
Comment 18•7 years ago
|
||
Comment on attachment 8911556 [details] Bug 1399265 - Avoid relayout when switching tab in devtools Approval Request Comment [Feature/Bug causing the regression]: n/a [User impact if declined]: Performance not improved. [Is this code covered by automated tests?]: - [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: n/a [Is the change risky?]: low risk (feature is for developers) [Why is the change risky/not risky?]: only one line of css changed [String changes made/needed]: n/a
Attachment #8911556 -
Flags: approval-mozilla-beta?
Comment 19•7 years ago
|
||
Comment on attachment 8911556 [details] Bug 1399265 - Avoid relayout when switching tab in devtools Fix a regression, taking it. Should be in 57b4
Attachment #8911556 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/45b0a92887e4
Comment 21•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #18) > [Is this code covered by automated tests?]: - > [Has the fix been verified in Nightly?]: yes > [Needs manual test from QE? If yes, steps to reproduce]: no Setting qe-verify- based on Honza's assessment on manual testing needs.
Flags: qe-verify-
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•2 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•