1946365 - 176.65 - 2.37% damp console.typing / damp simple.webconsole.reload.DAMP + 2 more (Windows) regression on Mon February 3 2025

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Treeherder Bug Filer]

Perfherder has detected a devtools performance regression from push fd544deaa5843da8f38108de96bd6a27e0c0be2e. 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)
177%	damp console.typing	windows11-64-shippable-qr	e10s fission stylo webrender	367.19 -> 1,015.84
41%	damp console.autocomplete	windows11-64-shippable-qr	e10s fission stylo webrender	360.63 -> 510.00
6%	damp console.autocomplete.longInput	windows11-64-shippable-qr	e10s fission stylo webrender	477.89 -> 506.05
2%	damp simple.webconsole.reload.DAMP	windows11-64-shippable-qr	e10s fission stylo webrender	186.85 -> 191.27

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 all of these tests on try with ./mach try perf --alert 43651

The following documentation link provides more information about this command.

For more information on performance sheriffing please see our FAQ.

If you have any questions, please do not hesitate to reach out to [e-mail].

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Huh, that's some massive regressions indeed. Possibly the popup creation time is worse? Can we have profiles of this?

Comment 3

[Alex Finder|PTO, back on March 31st]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Huh, that's some massive regressions indeed. Possibly the popup creation time is worse? Can we have profiles of this?

Generated the performance profiles for both the before and after jobs, they should be available soon. Let me know via :ni if they don't show up.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Ok so took a look at the profiles:

• Good: https://share.firefox.dev/4hwQfF3
• Bad: https://share.firefox.dev/4hAD32c

and what I can see is that there's a bunch of FillRgn that wasn't there before which is not totally unexpected given the change. That said, I suspect bug 1945894 will help here. If it doesn't restore the regression to previous values we can look into it more.

But also, I'm curious, what is this test measuring exactly? The difference in the profiles doesn't look so massive? (As in, the first item on the table is a 200% regression...)

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Ok so took a look at the profiles:

• Good: https://share.firefox.dev/4hwQfF3
• Bad: https://share.firefox.dev/4hAD32c

and what I can see is that there's a bunch of FillRgn that wasn't there before which is not totally unexpected given the change. That said, I suspect bug 1945894 will help here. If it doesn't restore the regression to previous values we can look into it more.

But also, I'm curious, what is this test measuring exactly? The difference in the profiles doesn't look so massive? (As in, the first item on the table is a 200% regression...)

focused the profiles on the console.typing test:

• https://share.firefox.dev/4hUCvnd : 409ms
• https://share.firefox.dev/4aWWrna : 1000ms

The test simulate typing a string char by char, triggering the autocomplete https://searchfox.org/mozilla-release/rev/8647b34112bf8f1ea2d630feffa0e863e82f79ca/testing/talos/talos/tests/devtools/addon/content/tests/webconsole/typing.js#20,50-66

const input = "abcdefghijklmnopqrst";
...
const test = runTest(TEST_NAME);

// Simulate typing in the input.
for (const char of Array.from(input)) {
  const onPopupOpened = jsterm.autocompletePopup.isOpen
    ? null
    : jsterm.autocompletePopup.once("popup-opened");
  const onAutocompleteUpdated = jsterm.once("autocomplete-updated");
  jsterm.insertStringAtCursor(char);
  // We need to trigger autocompletion update to show the popup.
  jsterm.props.autocompleteUpdate();
  await onAutocompleteUpdated;
  await onPopupOpened;
  await waitForTick();
}

test.done();

Comment 6

[Bob Hood [:bhood]]

Emilio, are you ok taking this one?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1946365 - Track already-clear region of transparent windows. r=#win-reviewers

Comment 8

[Emilio Cobos Álvarez (:emilio)]

https://perf.compare/compare-results?baseRev=8830a891ba3f945366e62c891c8b4e2a4f1fef38&baseRepo=try&newRev=51a4fd1fb42cb979443cbc344a319de11e279eb5&newRepo=try&framework=12

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Hmm, so this actually doesn't help that much because we're apparently always hiding and re-showing the autocomplete popup, rather than reusing it. That seems not ideal? Nicolas do you know what the setup here is? It seems avoiding that hide / show pairs would be a big improvement (even before this patch) since it avoids re-initializing all the compositor setup and such.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Since we're using jsterm.insertStringAtCursor(char); we don't see the "key stroke" as being an addition, and we do close the popup for each letter.
I can't really remember if that was on purpose, but this doesn't really match user behavior and we can probably dispatch key events instead (which will reuse the tooltip)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

But even when not in the benchmark I see more opening and closing than I'd expect. It seems every time the autocomplete list changes the popup is closed and re-opened, rather than it being reused which is what I'd expect. Is that expected? Should I move this to the DevTools component?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Using FillRect rather than FillRgn helps with the typing tests regardless, so we should probably do that: https://perf.compare/subtests-compare-results?baseRev=280b5320cdbc6135e41a081ebab1ee6d8f637763&baseRepo=try&newRev=027835e386c6b5b37228189b92de459b84025a0a&newRepo=try&framework=12&baseParentSignature=59287&newParentSignature=59287&filter_confidence=high

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

But even when not in the benchmark I see more opening and closing than I'd expect. It seems every time the autocomplete list changes the popup is closed and re-opened, rather than it being reused which is what I'd expect. Is that expected? Should I move this to the DevTools component?

I didn't looked to deep into this, I stopped when I saw hideTooltip was indeed called on each key stroke in the DAMP test.
We can probably keep this bug here so you can optimize the platform code, and I'll have a dedicated bug for the console autocomplete

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1946365 - Use FillRect to clear window. r=#win-reviewers

See comment 12 for perf numbers. If needed we could use it only if
!regionToClear.IsComplex(), but given this and the kind of regions we
usually deal with, FillRect is probably fine or even better perf wise.

It also avoids the region conversion which is nice.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d01c9ed6915
Track already-clear region of transparent windows. r=win-reviewers,rkraesig
https://hg.mozilla.org/integration/autoland/rev/7a2b52f98df7
Use FillRect to clear window. r=win-reviewers,rkraesig
https://hg.mozilla.org/integration/autoland/rev/ea196c69d404
apply code formatting via Lando

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Filed bug 1949051 for the devtools side improvements (re. comment 13)

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/4d01c9ed6915
https://hg.mozilla.org/mozilla-central/rev/7a2b52f98df7
https://hg.mozilla.org/mozilla-central/rev/ea196c69d404

Comment 18

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #15)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d01c9ed6915
Track already-clear region of transparent windows. r=win-reviewers,rkraesig
https://hg.mozilla.org/integration/autoland/rev/7a2b52f98df7
Use FillRect to clear window. r=win-reviewers,rkraesig
https://hg.mozilla.org/integration/autoland/rev/ea196c69d404
apply code formatting via Lando

Perfherder has detected a devtools performance change from push ea196c69d4048949cf14ef54f7d44716c1dddf18.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
64%	damp console.typing	windows11-64-shippable-qr	e10s fission stylo webrender	1,036.30 -> 378.05
33%	damp console.autocomplete	windows11-64-shippable-qr	e10s fission stylo webrender	553.38 -> 368.79
6%	damp console.autocomplete.longInput	windows11-64-shippable-qr	e10s fission stylo webrender	516.41 -> 486.58

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

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 44008

For more information on performance sheriffing please see our FAQ.