1352687 - HTMLInputElement constructor allocates nsTextEditorState which may then be immediately released when the type is changed

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Altogether this is 7% of this rather silly Speedometer innerHTML profile I'm looking at.
(Speedometer is rather useless as a DOM benchmark, since it measures largely just <input> element handling, but it may be good for JS, so getting DOM out from the profile is what I'm trying to do)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

We could possibly reuse nsTextEditorState objects, at least one. Then we'd use the reused one when creating an element, and put it back to cache when parser sets input element's type to some non-single-line-text-control.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8853682 [details] [diff] [review]
v1

baku, are you ok reviewing this?

nsTextEditorState is a bit weird thing, since it is initialize using 'new' when used with HTMLInputElement, but without 'new' when used with HTMLTextArea, since the latter always has such object attached to it. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a09031ba972e87aeb9217114d5a4690d146603a2

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8853682 [details] [diff] [review]
v1

Review of attachment 8853682 [details] [diff] [review]:
-----------------------------------------------------------------

We should consider the introduction of a CachedObject helper class if this issue is needed elsewhere.

::: dom/html/nsTextEditorState.cpp
@@ +1092,5 @@
> +    state->mSelectionRestoreEagerInit = false;
> +    state->mPlaceholderVisibility = false;
> +    state->mIsCommittingComposition = false;
> +    // When adding more member variable initializations here, add the same
> +    // also to the constructor.

What about introducing a Initialize(); and call this method in the CTOR and here?

::: dom/html/nsTextEditorState.h
@@ +149,5 @@
> +    mValue.reset();
> +    mCachedValue.Truncate();
> +    mValueBeingSet.Truncate();
> +    mTextCtrlElement = nullptr;
> +    MOZ_ASSERT(!mMutationObserver);

why do not call ::Initialize() here instead in the ::Construct?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

A reason for not having Initialize is that there are member variables like mInitializing and mEverInited, and those have nothing to do with this caching stuff. Construct as a method name might be ok.

And I wanted to keep the existing possibly faster creation when constructing a totally new object.

PrepareForReuse really just releases all the extra memory, which constructor doesn't need to do.
Better to avoid extra Truncate() calls and such. Remember, this is hot code.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe1d8e2e07f
try to recycle HTMLInputElement's nsTextEditorState, r=baku

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dbe1d8e2e07f