Closed
Bug 1352687
Opened 8 years ago
Closed 8 years ago
HTMLInputElement constructor allocates nsTextEditorState which may then be immediately released when the type is changed
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(1 file)
(deleted),
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
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
Comment 7•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•