32.73 - 3.32% damp console.objectexpand-many-instances / damp custom.jsdebugger.open-large-minified-file.full-selection.DAMP + 20 more (Linux, OSX, Windows) regression on Tue February 6 2024
Categories
(DevTools :: Object Inspector, defect, P2)
Tracking
(firefox-esr115 unaffected, firefox122 unaffected, firefox123 unaffected, firefox124 fixed, firefox125 fixed)
Tracking | Status | |
---|---|---|
firefox-esr115 | --- | unaffected |
firefox122 | --- | unaffected |
firefox123 | --- | unaffected |
firefox124 | --- | fixed |
firefox125 | --- | fixed |
People
(Reporter: nchevobbe, Assigned: nchevobbe)
References
(Regression)
Details
(Keywords: perf, perf-alert, regression)
Attachments
(2 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
phab-bot
:
approval-mozilla-beta+
|
Details | Review |
Perfherder has detected a devtools performance regression from push 79b38383448166074627506218757a9881ca5679. As author of one of the patches included in that push, we need your help to address this regression.
Regressions:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
33% | damp console.objectexpand-many-instances | linux1804-64-shippable-qr | e10s fission stylo webrender | 707.99 -> 939.70 |
32% | damp console.objectexpand-many-instances | linux1804-64-shippable-qr | e10s fission stylo webrender-sw | 725.48 -> 955.05 |
28% | damp console.objectexpand-many-instances | windows10-64-shippable-qr | e10s fission stylo webrender-sw | 574.48 -> 733.58 |
27% | damp console.objectexpand-many-instances | macosx1015-64-shippable-qr | e10s fission stylo webrender-sw | 646.62 -> 818.89 |
26% | damp console.objectexpand-many-instances | macosx1015-64-shippable-qr | e10s fission stylo webrender | 647.30 -> 815.79 |
26% | damp console.objectexpand-many-instances | windows10-64-shippable-qr | e10s fission stylo webrender | 580.71 -> 728.79 |
24% | damp custom.jsdebugger.open-large-minified-file.DAMP | linux1804-64-shippable-qr | e10s fission stylo webrender-sw | 960.34 -> 1,193.73 |
24% | damp custom.jsdebugger.open-large-minified-file.DAMP | linux1804-64-shippable-qr | e10s fission stylo webrender | 961.44 -> 1,192.38 |
24% | damp console.objectexpanded.close.DAMP | linux1804-64-shippable-qr | e10s fission stylo webrender-sw | 77.88 -> 96.45 |
24% | damp console.objectexpanded.close.DAMP | macosx1015-64-shippable-qr | e10s fission stylo webrender | 56.61 -> 69.96 |
... | ... | ... | ... | ... |
6% | damp console.bulklog | macosx1015-64-shippable-qr | e10s fission stylo webrender-sw | 1,092.47 -> 1,156.34 |
6% | damp custom.jsdebugger.open-large-minified-file.full-selection.DAMP | linux1804-64-shippable-qr | e10s fission stylo webrender-sw | 4,507.28 -> 4,764.67 |
5% | damp custom.jsdebugger.open-large-minified-file.full-selection.DAMP | linux1804-64-shippable-qr | e10s fission stylo webrender | 4,652.39 -> 4,863.22 |
3% | damp custom.jsdebugger.open-large-minified-file.full-selection.DAMP | macosx1015-64-shippable-qr | e10s fission stylo webrender | 4,053.10 -> 4,187.48 |
3% | damp custom.jsdebugger.open-large-minified-file.full-selection.DAMP | macosx1015-64-shippable-qr | e10s fission stylo webrender-sw | 4,057.07 -> 4,184.31 |
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.
If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.
You can run these tests on try with ./mach try perf --alert 41399
For more information on performance sheriffing please see our FAQ.
Assignee | ||
Comment 1•8 months ago
|
||
Identified Bug 1824671 as the offender. I pushed to try reverting all the patches from the bug and DAMP results indicates that the patch does have a pretty big impact: https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=47088f1cf29949f0372741c7879b0224abf936cb&originalSignature=4763542&newSignature=4763542&framework=12&application=firefox&originalRevision=3aa92917a1abe392a6781b074dc17625f8058a38&page=1&showOnlyImportant=1
Jonathan, any idea what might be happening? This is impacting the DevTools console and debugger performance tests
Comment 2•8 months ago
|
||
Set release status flags based on info from the regressing bug 1824671
Comment 3•8 months ago
|
||
:nchevobbe do you think you can set a priority/severity on this?
Comment 4•8 months ago
|
||
Nicolas, I was going to try profiling locally to investigate this, but attempting to run:
./mach talos-test --suite damp --subtests console.objectexpand-many-instances --cycles 1 --tppagecycles 1 --gecko-profile --gecko-profile-entries 100000000
(based on what I found at https://firefox-source-docs.mozilla.org/devtools/tests/performance-tests-damp.html) doesn't seem to work for me; I get a browser window with a DAMP header, but nothing happens.... presumably because:
07:20:22 INFO - PID 88153 | [damp-api] Retrieve the main DevTools loader
07:20:22 INFO - PID 88153 | [damp-api] Retrieve the DAMP runner and start the test
07:20:22 INFO - PID 88153 | [DampLoad helper] Register DampLoad actors
07:20:22 INFO - PID 88153 | TEST-UNEXPECTED-FAIL | damp | Unable to find any test matching 'console.objectexpand-many-instances'
07:20:22 INFO - PID 88153 | TEST-UNEXPECTED-FAIL | damp | Exception: can't access property "subtestEnd", this.TalosParentProfiler is undefined
What am I doing wrong here? Thanks!
Assignee | ||
Comment 5•8 months ago
|
||
does ./mach talos-test --suite damp-webconsole --subtest custom.webconsole --cycles 1 --tppagecycles 1 --gecko-profile --gecko-profile-entries 100000000
works better ? I don't think we can target onlly the objectexpand test. In the profile, there are markers for DAMP tests, so you should be able to find console.objectexpand-many-instances
and only focus on it
Comment 6•8 months ago
|
||
Aha, that looks much happier - thanks.
Comment 7•8 months ago
|
||
(Just FTR, it's actually --subtest console
that runs the set of tests that include objectexpand-many-instances
.)
Comparing profiles "before" and "after", it looks like bidi resolution may be taking slightly longer, perhaps due to some additional allocation churn; I'll look into whether we can eliminate some heap allocations that unicode-bidi is doing internally. But that doesn't seem like nearly enough to account for most of the regression here; the larger factor seems to be managing the bidi data attached to frames and ordering frames within the line.
So the question is why that's different, given that bug 1824671 didn't touch anything at that level, it only replaced the underlying implementation of the bidi algorithm.
I wondered if this is related to DevTools use of unicode-bidi values (or equivalent Unicode directional-override or embedding control characters) to manage its display in the presence of potential bidi content and/or an RTL interface? The new bidi implementation behaves slightly differently with respect to embeddings (it more closely follows the recommendations of UAX#9 section 5.2), which may result in a different structure of bidi continuations.
Assignee | ||
Comment 8•7 months ago
|
||
(In reply to Jonathan Kew [:jfkthame] from comment #7)
I wondered if this is related to DevTools use of unicode-bidi values (or equivalent Unicode directional-override or embedding control characters) to manage its display in the presence of potential bidi content and/or an RTL interface? The new bidi implementation behaves slightly differently with respect to embeddings (it more closely follows the recommendations of UAX#9 section 5.2), which may result in a different structure of bidi continuations.
Thanks for the investigation. So we have this loose CSS rule (added in Bug 1688168) that will apply to non-primitive objects we render in the console and debugger https://searchfox.org/mozilla-central/rev/1aaacaeb4fa3aca6837ecc157e43e947229ba8ce/devtools/client/shared/components/reps/reps.css#40-42
.objectBox * {
unicode-bidi: isolate;
}
Commenting this does bring the duration of the test down.
Jonathan, is there something we could do better in DevTools here?
Comment 9•7 months ago
|
||
Hmm, I'm not sure offhand.... can we be more selective about what elements really need the unicode-bidi rule applied? It makes sense to apply isolation to bits of data that have RTL characters in them, so that they don't end up affecting the ordering of the surrounding punctuation, etc., but it's unfortunate if we're forced to create an isolation run for every fragment even when it's not needed. That will be making the frame tree much more complex (and therefore expensive to process) than necessary.
Inspecting the console rendering of the example "أهلا".split("")
from bug 1688168, I'm seeing unicode-bidi: isolate
applied to the objectTitle
span that displays the description Array(4)
, and again to the inner objectLengthBubble
that wraps the (4)
within it. Then the arrayLeftBracket
span gets its own isolate
, as does each of the individual array elements (single-character strings, in this case), and the arrayRightBracket
span that follows. Finally there's an (empty) arrayProperties
span, again with unicode-bidi: isolate
applied.
The only ones of these isolate
properties that are necessary for the display here, afaics, are those on the actual array elements; everything else here is part of the devtools interface text and should simply flow in the interface direction (LTR), without needing to be broken into all these isolates. So for this example, at least, just .objectBox-string { unicode-bidi: isolate }
would be sufficient to prevent the RTL strings disrupting the overall rendering direction, without needing to apply it to every other descendant of an .objectBox
.
I expect there are other classes besides .objectBox-string
that might contain arbitrary (potentially-bidi) strings, and therefore need isolation to avoid disrupting the interface, so refining this may be non-trivial.... as an experiment, I built with a simple patch:
diff --git a/devtools/client/shared/components/reps/reps.css b/devtools/client/shared/components/reps/reps.css
--- a/devtools/client/shared/components/reps/reps.css
+++ b/devtools/client/shared/components/reps/reps.css
@@ -37,7 +37,10 @@
white-space: pre-wrap;
}
-.objectBox * {
+.objectBox-string,
+.objectBox-symbol,
+.objectBox-text,
+.objectBox-textNode {
unicode-bidi: isolate;
}
and while this displays the "أهلا".split("")
example fine, it results in a number of failures in devtools/client/webconsole/test/browser/browser_webconsole_bidi_string_isolation.js, so clearly there's more to be considered. But maybe this is a starting point? If we can limit the use of unicode-bidi: isolate
to the actual fragments of data that potentially need it, I'd expect a significant performance gain.
The other thing I wondered is whether there's an opportunity, when objects are being written to the console or debugger, for us to check whether the string contains any RTL characters, and only then set the unicode-bidi property on the span? Or would doing such a check/conditional styling itself be too expensive? Currently we're applying this property all over the place, even when there's no RTL content anywhere, which is a bit sad (and costly).
Assignee | ||
Comment 10•7 months ago
|
||
Thanks for the investigation Jonathan
(In reply to Jonathan Kew [:jfkthame] from comment #9)
Hmm, I'm not sure offhand.... can we be more selective about what elements really need the unicode-bidi rule applied? […] If we can limit the use of
unicode-bidi: isolate
to the actual fragments of data that potentially need it, I'd expect a significant performance gain.
We can definitely do that, and thanks for trying, I'll do something in that vein, making sure we pass the test covering this
The other thing I wondered is whether there's an opportunity, when objects are being written to the console or debugger, for us to check whether the string contains any RTL characters, and only then set the unicode-bidi property on the span? Or would doing such a check/conditional styling itself be too expensive? Currently we're applying this property all over the place, even when there's no RTL content anywhere, which is a bit sad (and costly).
Not sure, is there an API that exists so we could check that? Otherwise I guess we can use a regex to find chars from specific ranges, not sure how fast/slow this would perform.
Not directly related to this performance regression, but should we open another bug to investigate potential real word usage regression because of Bug 1824671 ?
Assignee | ||
Comment 11•7 months ago
|
||
The CSS property was applied broadly on all element with
a objectBox parent, which was bad for performance.
Updated•7 months ago
|
Comment 12•7 months ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)
The other thing I wondered is whether there's an opportunity, when objects are being written to the console or debugger, for us to check whether the string contains any RTL characters, and only then set the unicode-bidi property on the span?
Not sure, is there an API that exists so we could check that? Otherwise I guess we can use a regex to find chars from specific ranges, not sure how fast/slow this would perform.
We have something on the Gecko/C++ side -- there's a HasRTLChars
function (see intl/unicharutil/util/nsBidiUtils.h) that checks whether a buffer of text contains any bidi chars. I don't know if there's currently anything like that in JS, though presumably we could expose a helper (e.g. on Services.intl) to provide the functionality.
Not directly related to this performance regression, but should we open another bug to investigate potential real word usage regression because of Bug 1824671 ?
We have a couple of other related perf-regression bugs on file that I'm currently looking into, so I think this is adequately covered.
Comment 13•7 months ago
|
||
(In reply to Jonathan Kew [:jfkthame] from comment #12)
We have something on the Gecko/C++ side -- there's a
HasRTLChars
function (see intl/unicharutil/util/nsBidiUtils.h) that checks whether a buffer of text contains any bidi chars. I don't know if there's currently anything like that in JS, though presumably we could expose a helper (e.g. on Services.intl) to provide the functionality.
FWIW, I see that pdf.js does a basic check along these lines at https://searchfox.org/mozilla-central/rev/97feebcab27f1a92e70ceacaa77211e9eaba0e6e/js/src/octane/pdfjs.js#32672-32690, but it'd be far preferable to rely on a maintained API rather than an ad hoc regex or range checks like this. I notice the pdf.js code doesn't check for bidi control codes, for example -- perhaps they're not relevant for its use case -- but also doesn't deal with the RTL ranges in supplementary planes at all. If we do pursue this for devtools purposes, let's make sure we do it more thoroughly.
Assignee | ||
Comment 14•7 months ago
|
||
(In reply to Jonathan Kew [:jfkthame] from comment #12)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)
The other thing I wondered is whether there's an opportunity, when objects are being written to the console or debugger, for us to check whether the string contains any RTL characters, and only then set the unicode-bidi property on the span?
Not sure, is there an API that exists so we could check that? Otherwise I guess we can use a regex to find chars from specific ranges, not sure how fast/slow this would perform.
We have something on the Gecko/C++ side -- there's a
HasRTLChars
function (see intl/unicharutil/util/nsBidiUtils.h) that checks whether a buffer of text contains any bidi chars. I don't know if there's currently anything like that in JS, though presumably we could expose a helper (e.g. on Services.intl) to provide the functionality.
Ah that's great, I'll see if I can use it and how impactful this may be
Not directly related to this performance regression, but should we open another bug to investigate potential real word usage regression because of Bug 1824671 ?
We have a couple of other related perf-regression bugs on file that I'm currently looking into, so I think this is adequately covered.
Perfect then :)
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 17•7 months ago
|
||
I do have some test results.
With a patch where I'm restricting the selectors on which we apply unicode-bidi: isolate
, I'm getting a 4% improvement on console.objectexpand-many-instances
(perfherder )
With a patch where I check if we have RTL chars, and set a specific class we target to apply unicode-bidi: isolate
, I'm getting a 31.6% improvement on console.objectexpand-many-instances
(perfherder). Profiling the DAMP run, I couldn't even see the impact of the new method I exposed to check the RTL chars, so this sounds like a nice solution to me.
Comment 18•7 months ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #17)
With a patch where I check if we have RTL chars, and set a specific class we target to apply
unicode-bidi: isolate
, I'm getting a 31.6% improvement onconsole.objectexpand-many-instances
Nice!
Before completely declaring victory here, a couple more thoughts that might need to be taken into account:
(a) does the devtools console always use LTR directionality, or do we have localized versions where the devtools interface is RTL overall (but some values might be LTR), in which case the situations where isolate
is needed would be inverted, more or less; and
(b) is this applicable to objects (e.g. in the debugger) where the user might edit what's displayed, and if they insert RTL characters on the fly, will things be updated accordingly?
Assignee | ||
Comment 19•7 months ago
|
||
(In reply to Jonathan Kew [:jfkthame] from comment #18)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #17)
With a patch where I check if we have RTL chars, and set a specific class we target to apply
unicode-bidi: isolate
, I'm getting a 31.6% improvement onconsole.objectexpand-many-instances
Nice!
Before completely declaring victory here, a couple more thoughts that might need to be taken into account:
(a) does the devtools console always use LTR directionality, or do we have localized versions where the devtools interface is RTL overall (but some values might be LTR), in which case the situations whereisolate
is needed would be inverted, more or less; and
we force ltr direction for a few elements in the UI, so we should be good here
(b) is this applicable to objects (e.g. in the debugger) where the user might edit what's displayed, and if they insert RTL characters on the fly, will things be updated accordingly?
I'm not sure I understand the use case, could you give me some steps for what you have in mind? But I think everything is good, those "Reps" (for representation of objects) are only rendered once, for specific objects. If the underlying JS objects are modified, the whole Reps will be re-rendered (and so we'll check if there are RTL chars and apply the right class in such case).
Comment 20•7 months ago
|
||
Set release status flags based on info from the regressing bug 1824671
Comment 21•7 months ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #19)
I'm not sure I understand the use case, could you give me some steps for what you have in mind? But I think everything is good, those "Reps" (for representation of objects) are only rendered once, for specific objects. If the underlying JS objects are modified, the whole Reps will be re-rendered (and so we'll check if there are RTL chars and apply the right class in such case).
I wasn't sure if these could be directly editable in the UI, but given what you describe, it sounds like this shouldn't be an issue.
Assignee | ||
Updated•7 months ago
|
Assignee | ||
Comment 22•7 months ago
|
||
(In reply to Jonathan Kew [:jfkthame] from comment #21)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #19)
I'm not sure I understand the use case, could you give me some steps for what you have in mind? But I think everything is good, those "Reps" (for representation of objects) are only rendered once, for specific objects. If the underlying JS objects are modified, the whole Reps will be re-rendered (and so we'll check if there are RTL chars and apply the right class in such case).
I wasn't sure if these could be directly editable in the UI, but given what you describe, it sounds like this shouldn't be an issue.
Right, the user can't edit those, so we should be fine
Updated•7 months ago
|
Comment 23•7 months ago
|
||
Comment 24•7 months ago
|
||
Backed out for causing dt failures @ devtools/client/jsonview/test/browser_jsonview_copy_json.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/29502e93e5a1dafb93c2d85c64676fe8b5ce1cb8
Assignee | ||
Updated•7 months ago
|
Comment 25•7 months ago
|
||
Comment 26•7 months ago
|
||
bugherder |
Updated•7 months ago
|
Assignee | ||
Comment 27•7 months ago
|
||
The CSS property was applied broadly on all element with
a objectBox parent, which was bad for performance.
We now use Services.intl.stringHasRTLChars
to detect if the string we're going
to render has some RTL chars, and in such case, we add a has-rtl-char
class
to the element so we can target it in CSS.
Original Revision: https://phabricator.services.mozilla.com/D202200
Updated•7 months ago
|
Comment 28•7 months ago
|
||
Uplift Approval Request
- String changes made/needed: -
- Needs manual QE test: no
- Explanation of risk level: DevTools only bug, impacting JS Objects containing RTL chars, covered by mochitest
- Risk associated with taking this patch: low
- Is Android affected?: no
- Code covered by automated testing: yes
- Fix verified in Nightly: no
- User impact if declined: DevTools Console and Debugger will be slower in 124 than it was in 123
- Steps to reproduce for manual QE testing: -
Updated•7 months ago
|
Updated•7 months ago
|
Comment 29•7 months ago
|
||
uplift |
Assignee | ||
Comment 30•7 months ago
|
||
== Change summary for alert #41580 (as of Mon, 26 Feb 2024 16:21:32 GMT) ==
Improvements:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
30% | damp console.objectexpand-many-instances | linux1804-64-shippable-qr | e10s fission stylo webrender | 936.48 -> 658.42 |
29% | damp console.objectexpand-many-instances | linux1804-64-shippable-qr | e10s fission stylo webrender-sw | 940.46 -> 665.85 |
28% | damp console.objectexpanded.close.DAMP | linux1804-64-shippable-qr | e10s fission stylo webrender | 95.36 -> 68.67 |
27% | damp console.objectexpanded.close.DAMP | windows10-64-shippable-qr | e10s fission stylo webrender-sw | 77.57 -> 56.89 |
26% | damp console.objectexpanded.close.DAMP | linux1804-64-shippable-qr | e10s fission stylo webrender-sw | 95.17 -> 70.18 |
... | ... | ... | ... | ... |
7% | damp console.autocomplete | linux1804-64-shippable-qr | e10s fission stylo webrender | 777.99 -> 726.74 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=41580
Description
•