Closed Bug 1352687 Opened 3 years ago Closed 3 years ago

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

Categories

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

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(1 file)

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)
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.
Assignee: nobody → bugs
Attached patch v1Splinter Review
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
Attachment #8853682 - Flags: review?(amarchesini)
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?
Attachment #8853682 - Flags: review?(amarchesini) → review+
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.
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe1d8e2e07f
try to recycle HTMLInputElement's nsTextEditorState, r=baku
https://hg.mozilla.org/mozilla-central/rev/dbe1d8e2e07f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.