1597679 - 7.86% raptor-tp6-twitch-firefox-cold fcp (windows7-32-shippable) regression on push 6cc550e9edcde95eaba889aa340da4f9781dfb09 (Sun November 3 2019)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Raptor has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=6cc550e9edcde95eaba889aa340da4f9781dfb09

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

8% raptor-tp6-twitch-firefox-cold fcp windows7-32-shippable opt 81.67 -> 88.08

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=23983

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a Treeherder page showing the Raptor jobs in a pushlog format.

To learn more about the regressing test(s) or reproducing them, please see: https://wiki.mozilla.org/TestEngineering/Performance/Raptor

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/TestEngineering/Performance/Talos/RegressionBugsHandling

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, backing out the patch will break my main working patch which are really big (bug 970802). So, I'll take a look right now.

If this occurs only on Windows, I guess that this is caused by the difference between platforms about heavy string usage (bug 1398572 comment 1).

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, in the content process, TextControlState does not appear. but in the main process it appears. However, if the latter is the cause, all talos tests should be regressed.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

One possible reason in the main process is, removing AutoScriptBlocker causes redundant reflow due to bug 1595425.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Anyway, I have some idea about the bottle necks. I'll try to write patches.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I believe that the fix of bug1595425 will improve the score mostly. However, I have some patches to optimize the path. So, please don't close this until I land them.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

In my investigation, the score includes chrome UI initialization time, and the test is really short that's the reason why the regression appears in big percentage if there is no other performance regression bugs.

So, I think that any improvements of setting value of <input> element helps this regression fix (perhaps, fixing bug 1598327 also might affect this), but I guess that we cannot take 100% of the score back anyway because some redundant path is now run multiple times maybe, but it's necessary cost for removing the script-blocker hack.

Anyway, I'll post some optimization patches soon, although they might not improve the score like this test result:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=37b02bc76eeed3cbb5e575ec364efa049b0237bb&newProject=try&newRevision=d17039f7ae16c8c7cc1db15e5c8ccbb11b618c6c&framework=10&pageTitle=Perfherder+Compare+Revisions+%28part+0+vs.+part+3%29

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1597679 - part 1: Cache multiple `TextControlState` instances for reuse r=ehsan!

Previously, we cache only one TextControlState instance to reuse. However,
HTMLTextAreaElement also started to allocate it in the heap rather than
a part of its instance. Therefore, we should have more room to cache the
instances for saving the allocation cost.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1597679 - part 2: Make `nsITextControlElement` inherit `nsGenericHTMLFormElementWithState` r=smaug!

Sub classes of nsITextControlElement are only HTMLInputElement and
HTMLTextAreaElement. And both base class is
nsGenericHTMLFormElementWithState. Therefore, we can make
nsITextControlElement inherit nsGenericHTMLFormElementWithState and
make HTMLInputElement and HTMLTextAreaElement inherit
nsITextControlElement. Then, we can get rid of a lot of QI between
nsINode/nsIContent/Element and nsITextControlElement (and note that
some of them in a hot path).

Additionally, this patch renames nsITextControlElement to
mozilla::TextControlElement.

Depends on D54326

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1597679 - part 3: Create methods to compare given string with values of `TextControlState`, `nsTextControlFrame`, `HTMLInputElement` and `HTMLTextAreaElement` r=smaug!

For avoiding unnecessary copy of string buffer only for comparing setting
value and current value, especially with nsAutoString, this patch
creates *Equals() methods for every class.

And also this avoids to call nsContentUtils::PlatformToDOMLineBreaks() in
most paths.

Depends on D54330

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4a156cf28ff
part 1: Cache multiple `TextControlState` instances for reuse r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/90a172eff2de
part 2: Make `nsITextControlElement` inherit `nsGenericHTMLFormElementWithState` r=smaug
https://hg.mozilla.org/integration/autoland/rev/6a73b58e0db4
part 3: Create methods to compare given string with values of `TextControlState`, `nsTextControlFrame`, `HTMLInputElement` and `HTMLTextAreaElement` r=smaug

Comment 11

[Cristina Coroiu [:ccoroiu]]

Backed out 3 changesets (bug 1597679) for Android debug build bustage at build/src/dom/base/nsContentAreaDragDrop.cpp

Backout: https://hg.mozilla.org/integration/autoland/rev/46acccbd937dacc4251e11915ede38b9bbb5e202

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6a73b58e0db4af86b3542eb6868eb2422fbeea56

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=277867860&repo=autoland&lineNumber=25776

[task 2019-11-24T06:14:58.547Z] 06:14:58 INFO - dom/bindings/UnifiedBindings13.o
[task 2019-11-24T06:14:58.548Z] 06:14:58 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/bindings'
[task 2019-11-24T06:14:59.664Z] 06:14:59 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/base'
[task 2019-11-24T06:14:59.664Z] 06:14:59 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ --target=i686-linux-android -o Unified_cpp_dom_base7.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -fstack-protector-strong -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include/i686-linux-android -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include -gcc-toolchain /builds/worker/fetches/android-ndk/toolchains/x86-4.9/prebuilt/linux-x86_64 -D__ANDROID_API__=16 -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -fno-aligned-new -fno-short-enums -fno-exceptions -stdlib=libstdc++ -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++/include -I/builds/worker/fetches/android-ndk/sources/android/support/include -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++abi/include -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/obj-firefox/dom/base -I/builds/worker/workspace/build/src/dom/battery -I/builds/worker/workspace/build/src/dom/events -I/builds/worker/workspace/build/src/dom/media -I/builds/worker/workspace/build/src/dom/network -I/builds/worker/workspace/build/src/caps -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/dom/file -I/builds/worker/workspace/build/src/dom/geolocation -I/builds/worker/workspace/build/src/dom/html -I/builds/worker/workspace/build/src/dom/ipc -I/builds/worker/workspace/build/src/dom/storage -I/builds/worker/workspace/build/src/dom/svg -I/builds/worker/workspace/build/src/dom/u2f -I/builds/worker/workspace/build/src/dom/xml -I/builds/worker/workspace/build/src/dom/xslt/xpath -I/builds/worker/workspace/build/src/dom/xul -I/builds/worker/workspace/build/src/extensions/permissions -I/builds/worker/workspace/build/src/gfx/2d -I/builds/worker/workspace/build/src/image -I/builds/worker/workspace/build/src/js/xpconnect/loader -I/builds/worker/workspace/build/src/js/xpconnect/src -I/builds/worker/workspace/build/src/js/xpconnect/wrappers -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/layout/forms -I/builds/worker/workspace/build/src/layout/generic -I/builds/worker/workspace/build/src/layout/style -I/builds/worker/workspace/build/src/layout/svg -I/builds/worker/workspace/build/src/layout/xul -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/netwerk/url-classifier -I/builds/worker/workspace/build/src/security/manager/ssl -I/builds/worker/workspace/build/src/widget -I/builds/worker/workspace/build/src/xpcom/ds -I/builds/worker/workspace/build/src/netwerk/sctp/datachannel -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Oz -mno-outline -fno-omit-frame-pointer -funwind-tables -Werror -Wno-error=shadow -MD -MP -MF .deps/Unified_cpp_dom_base7.o.pp Unified_cpp_dom_base7.cpp
[task 2019-11-24T06:14:59.664Z] 06:14:59 INFO - In file included from Unified_cpp_dom_base7.cpp:2:
[task 2019-11-24T06:14:59.664Z] 06:14:59 ERROR - /builds/worker/workspace/build/src/dom/base/nsContentAreaDragDrop.cpp:516:10: error: unknown type name 'TextControlElement'; did you mean 'mozilla::TextControlElement'?
[task 2019-11-24T06:14:59.664Z] 06:14:59 INFO - RefPtr<TextControlElement> textControlElement =
[task 2019-11-24T06:14:59.664Z] 06:14:59 INFO - ^~~~~~~~~~~~~~~~~~
[task 2019-11-24T06:14:59.664Z] 06:14:59 INFO - mozilla::TextControlElement
[task 2019-11-24T06:14:59.664Z] 06:14:59 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/TextControlElement.h:29:7: note: 'mozilla::TextControlElement' declared here
[task 2019-11-24T06:14:59.664Z] 06:14:59 INFO - class TextControlElement : public nsGenericHTMLFormElementWithState {
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - ^
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - In file included from Unified_cpp_dom_base7.cpp:2:
[task 2019-11-24T06:14:59.666Z] 06:14:59 ERROR - /builds/worker/workspace/build/src/dom/base/nsContentAreaDragDrop.cpp:517:7: error: use of undeclared identifier 'TextControlElement'; did you mean 'mozilla::TextControlElement'?
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - TextControlElement::GetTextControlElementFromEditingHost(editingElement);
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - ^~~~~~~~~~~~~~~~~~
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - mozilla::TextControlElement
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/TextControlElement.h:29:7: note: 'mozilla::TextControlElement' declared here
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - class TextControlElement : public nsGenericHTMLFormElementWithState {
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - ^
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - 2 errors generated.
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - /builds/worker/workspace/build/src/config/rules.mk:785: recipe for target 'Unified_cpp_dom_base7.o' failed
[task 2019-11-24T06:14:59.666Z] 06:14:59 ERROR - make[4]: *** [Unified_cpp_dom_base7.o] Error 1
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/base'
[task 2019-11-24T06:14:59.666Z] 06:14:59 INFO - make[4]: *** Waiting for unfinished jobs...

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d73c4e648588
part 1: Cache multiple `TextControlState` instances for reuse r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/8a0c0ad101ab
part 2: Make `nsITextControlElement` inherit `nsGenericHTMLFormElementWithState` r=smaug
https://hg.mozilla.org/integration/autoland/rev/19ec078199ad
part 3: Create methods to compare given string with values of `TextControlState`, `nsTextControlFrame`, `HTMLInputElement` and `HTMLTextAreaElement` r=smaug

Comment 13

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/d73c4e648588
https://hg.mozilla.org/mozilla-central/rev/8a0c0ad101ab
https://hg.mozilla.org/mozilla-central/rev/19ec078199ad

Comment 14

[Geoff Lankow (:darktrojan)]

I'm sure there was no intended change in behaviour, but I'm seeing textarea .value returning with \r and \r\n where previously there was only \n. Trying to figure out the circumstances as this doesn't happen if I just load data:text/html,<textarea/> in Nightly.

Comment 15

[(no longer active)]

(In reply to Geoff Lankow (:darktrojan) from comment #14)

I'm sure there was no intended change in behaviour, but I'm seeing textarea .value returning with \r and \r\n where previously there was only \n. Trying to figure out the circumstances as this doesn't happen if I just load data:text/html,<textarea/> in Nightly.

Sounds like a regression. Do you mind filing please with a STR? Thanks!

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, maybe, only in this path, we could include \r as-is.
https://searchfox.org/mozilla-central/rev/0678172d5b5c681061b904c776b668489e3355b0/editor/libeditor/TextEditSubActionHandler.cpp#843-844

Comment 17

[Geoff Lankow (:darktrojan)]

The character's coming from EventUtils.synthesizeKey("\r") in a Mochitest, if that helps.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #16)

Ah, maybe, only in this path, we could include \r as-is.
https://searchfox.org/mozilla-central/rev/0678172d5b5c681061b904c776b668489e3355b0/editor/libeditor/TextEditSubActionHandler.cpp#843-844

Hmm, this must be wrong. I cannot reproduce it with this path.

(In reply to Geoff Lankow (:darktrojan) from comment #17)

The character's coming from EventUtils.synthesizeKey("\r") in a Mochitest, if that helps.

Indeed, that's possible case, but it won't occur in real case. (FYI: it does not emulate Enter key press, instead, emulating odd key which produces \r, but ASCII control characters are filtered in each widget at handling native key event.)

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Old best score is around 70, but now, around 78. I have no idea to make the score better without big change (unless there is another regression at same time). So, I believe that the current score is necessary cost at least with current our editor design because I don't see TextControlState in the profile except nsString allocation cost. If we optimized deletion and insertion of <input> for XUL, the score must have become better. But it's not so important in editor module because we need to implement beforeinput event as soon as possible for web-compat (this regression reason is proparation for beforeinput).

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And according to the meaning of the test and profile, it's damage of startup performance of XUL UI, but it's really small since we don't get any alerts on the other platforms. Thus, optimization for the regression is really difficult.