Bugzilla

BugBot [:suhaib / :marco/ :calixte]

Assignee

Comment 1

•

4 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

•

4 months ago

Set release status flags based on info from the regressing bug 1824671

status-firefox122: --- → unaffected

status-firefox123: --- → unaffected

status-firefox124: --- → affected

status-firefox-esr115: --- → unaffected

Dianna Smith [:diannaS] - on Leave, back on August 9th

Comment 3

•

4 months ago

:nchevobbe do you think you can set a priority/severity on this?

Flags: needinfo?(nchevobbe)

Comment 4

•

4 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!

Flags: needinfo?(jfkthame)

Assignee

Comment 5

•

4 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

Flags: needinfo?(nchevobbe)

Comment 6

•

4 months ago

Aha, that looks much happier - thanks.

Comment 7

•

4 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

•

4 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

•

4 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

•

4 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

•

4 months ago

Attached file Bug 1879806 - [devtools] Only apply unicode-bidi: isolate to relevant elements. r=#devtools-reviewers. — Details

The CSS property was applied broadly on all element with
a objectBox parent, which was bad for performance.

Updated

•

4 months ago

Assignee: nobody → nchevobbe

Status: NEW → ASSIGNED

Comment 12

•

4 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

•

4 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

•

4 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)

(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.

So I'm trying to do that, but my C++ is quite poor and i'm hitting a compilation error.

Here's what I did :

diff --git a/dom/base/IntlUtils.cpp b/dom/base/IntlUtils.cpp
--- a/dom/base/IntlUtils.cpp
+++ b/dom/base/IntlUtils.cpp
@@ -9,6 +9,7 @@
 #include "mozilla/dom/ToJSValue.h"
 #include "mozilla/intl/LocaleService.h"
 #include "mozIMozIntl.h"
+#include "nsBidiUtils.h"
 #include "nsContentUtils.h"
 
 namespace mozilla::dom {
@@ -92,4 +93,8 @@ bool IntlUtils::IsAppLocaleRTL() {
   return intl::LocaleService::GetInstance()->IsAppLocaleRTL();
 }
 
+bool IntlUtils::IncludesRTLChars(const nsAutoString& aStr) {
+  return HasRTLChars(aStr);
+}
+
 }  // namespace mozilla::dom
diff --git a/dom/base/IntlUtils.h b/dom/base/IntlUtils.h
--- a/dom/base/IntlUtils.h
+++ b/dom/base/IntlUtils.h
@@ -41,6 +41,8 @@ class IntlUtils final : public nsISuppor
 
   bool IsAppLocaleRTL();
 
+  bool IncludesRTLChars(const nsAutoString& aStr);
+
  private:
   ~IntlUtils();
 
diff --git a/dom/webidl/IntlUtils.webidl b/dom/webidl/IntlUtils.webidl
--- a/dom/webidl/IntlUtils.webidl
+++ b/dom/webidl/IntlUtils.webidl
@@ -80,4 +80,9 @@ interface IntlUtils {
    *  - `intl.l10n.pseudo`
    */
   boolean isAppLocaleRTL();
+
+  /**
+   * Helper function to determine if passed string has RTL character.
+   */
+  boolean hasRTLChars(UTF8String str);
 };

and this is the error I get:

 0:07.69 In file included from UnifiedBindings11.cpp:132:
 0:07.69 ./IntlUtilsBinding.cpp:880:36: error: no member named 'HasRTLChars' in 'mozilla::dom::IntlUtils'
 0:07.69   880 |   bool result(MOZ_KnownLive(self)->HasRTLChars(Constify(arg0)));
 0:07.69       |               ~~~~~~~~~~~~~~~~~~~  ^
 0:09.56 1 error generated.
 0:09.57 make[4]: *** [UnifiedBindings11.o] Error 1
 0:09.57 make[3]: *** [dom/bindings/target-objects] Error 2
 0:09.58 make[3]: *** Waiting for unfinished jobs....
 0:11.04 make[2]: *** [compile] Error 2
 0:11.04 make[1]: *** [default] Error 2
 0:11.04 make: *** [build] Error 2

Would you know what I'm doing wrong?

Comment hidden (obsolete)

Assignee

Comment 17

•

4 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

•

4 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 on console.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?

BugBot [:suhaib / :marco/ :calixte]

Assignee

Comment 19

•

4 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 on console.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

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

•

4 months ago

Set release status flags based on info from the regressing bug 1824671

status-firefox125: --- → affected

Comment 21

•

4 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

•

4 months ago

Depends on: 1881078

Assignee

Updated

•

4 months ago

Component: Internationalization → Object Inspector

Product: Core → DevTools

Julian Descottes [:jdescottes]

Assignee

Comment 22

•

4 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

•

4 months ago

Severity: -- → S3

Priority: -- → P2

Pulsebot

Comment 23

•

4 months ago

Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3475061cb5df
[devtools] Only apply unicode-bidi: isolate to relevant elements. r=devtools-reviewers,ochameau.

Sandor Molnar[:smolnar]

Comment 24

•

4 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

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_copy_json.js | There must be proper JSON displayed - Got "nameInvalid object", expected "name"value""

Flags: needinfo?(nchevobbe)

Cristina Horotan [:chorotan]

Assignee

Updated

•

4 months ago

Flags: needinfo?(nchevobbe)

Pulsebot

Comment 25

•

4 months ago

Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35fcff350152
[devtools] Only apply unicode-bidi: isolate to relevant elements. r=devtools-reviewers,ochameau.

Comment 26

•

4 months ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/35fcff350152

Status: ASSIGNED → RESOLVED

Closed: 4 months ago

status-firefox125: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 125 Branch

Dianna Smith [:diannaS] - on Leave, back on August 9th

Updated

•

4 months ago

status-firefox124: affected → disabled

Assignee

Comment 27

•

4 months ago

Attached file Bug 1879806 - [devtools] Only apply unicode-bidi: isolate to relevant elements. r=#devtools-reviewers. — Details

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

•

4 months ago

Attachment #9387863 - Flags: approval-mozilla-beta?

Comment 28

•

4 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

•

4 months ago

Attachment #9387863 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Lando Automation

Updated

•

4 months ago

status-firefox124: disabled → fixed

Pulsebot

Comment 29

•

4 months ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/e3f50b9d672d