Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(67)

Issue 1355013002: Speed up updating inner value of HTMLTextFormControlElements. (Closed)

Created:
5 years, 3 months ago by sof
Modified:
5 years, 2 months ago
CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Speed up updating inner value of HTMLTextFormControlElements. Upon setting the inner string value of an element like <textarea/>, we append a solitary trailing <br> if the new value ends with a line break character (for layout and content editable purposes.) Upon replacing that string value with another, check if the shadowed inner element being updated has such a trailing <br> element and remove it first, before proceeding with setting the new value. Doing so enables fast replacement of text nodes rather than invoking the general path of replacing a set of siblings with a text node. This is of most benefit with Oilpan enabled, as it reduces the heap allocations made when handling this update operation. For a deep-loop microbenchmark like blink_perf.DOM.textarea-dom, a 10% speedup is observed (linux, windows). Linux64-chrome numbers on textarea-dom: - Oilpan enabled: + ToT: 14.6 runs/s + CL: 16.6 runs/s - Oilpan disabled: + ToT: 16.5 runs/s + CL: 16.9 runs/s R=tkent,haraken,esprehn BUG= Committed: https://crrev.com/175ce93e9e4dd69a6191d28f7c21a63f2541f006 Cr-Commit-Position: refs/heads/master@{#350989}

Patch Set 1 #

Total comments: 2

Patch Set 2 : early return #

Patch Set 3 : adjust textarea-dom to work over varying strings #

Total comments: 2

Patch Set 4 : rebased #

Total comments: 3

Patch Set 5 : issue handleTextFormControlChanged() post-update #

Total comments: 2

Patch Set 6 : Pare down comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -8 lines) Patch
M third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp View 1 2 3 4 5 1 chunk +16 lines, -8 lines 0 comments Download

Messages

Total messages: 31 (7 generated)
haraken
Thanks for looking at this! This change makes sense to me. +yosin, tkent, keishi
5 years, 3 months ago (2015-09-20 00:31:27 UTC) #2
yosin_UTC9
Thanks for paying attention to TEXTAREA! Rather than adding kludge, I would like to change ...
5 years, 3 months ago (2015-09-20 12:42:40 UTC) #3
sof
This is addressing a perf blocker for Oilpan, not reworking HTMLTextFormControlElement internals for longer-term benefits. ...
5 years, 3 months ago (2015-09-20 13:05:05 UTC) #4
esprehn
Not lgtm. We should not do different things in terms of dom mutation in oilpan ...
5 years, 3 months ago (2015-09-21 03:37:25 UTC) #5
sof
On 2015/09/21 03:37:25, esprehn wrote: > Not lgtm. We should not do different things in ...
5 years, 3 months ago (2015-09-21 05:26:49 UTC) #6
esprehn
On 2015/09/21 at 05:26:49, sigbjornf wrote: > On 2015/09/21 03:37:25, esprehn wrote: > > Not ...
5 years, 3 months ago (2015-09-21 08:20:43 UTC) #7
sof
On 2015/09/21 08:20:43, esprehn wrote: > On 2015/09/21 at 05:26:49, sigbjornf wrote: > > On ...
5 years, 3 months ago (2015-09-21 08:34:49 UTC) #8
esprehn
On 2015/09/21 at 08:34:49, sigbjornf wrote: > ... > > If it is in a ...
5 years, 3 months ago (2015-09-21 08:49:24 UTC) #9
sof
On 2015/09/21 08:49:24, esprehn wrote: > On 2015/09/21 at 08:34:49, sigbjornf wrote: > > ...
5 years, 3 months ago (2015-09-21 08:57:00 UTC) #10
sof
If textarea-dom is adjusted slightly to work over strings that vary, e.g., for (var i ...
5 years, 3 months ago (2015-09-21 11:57:07 UTC) #11
sof
On 2015/09/21 11:57:07, sof wrote: > If textarea-dom is adjusted slightly to work over strings ...
5 years, 3 months ago (2015-09-22 10:46:56 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1355013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1355013002/40001
5 years, 3 months ago (2015-09-22 20:23:07 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 3 months ago (2015-09-22 21:59:23 UTC) #16
tkent
https://codereview.chromium.org/1355013002/diff/40001/PerformanceTests/DOM/textarea-dom.html File PerformanceTests/DOM/textarea-dom.html (right): https://codereview.chromium.org/1355013002/diff/40001/PerformanceTests/DOM/textarea-dom.html#newcode19 PerformanceTests/DOM/textarea-dom.html:19: nodes.push(document.createTextNode('A quick brown fox jumps over the ' + ...
5 years, 3 months ago (2015-09-24 03:16:40 UTC) #18
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1355013002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1355013002/60001
5 years, 2 months ago (2015-09-25 08:50:39 UTC) #20
sof
https://codereview.chromium.org/1355013002/diff/40001/PerformanceTests/DOM/textarea-dom.html File PerformanceTests/DOM/textarea-dom.html (right): https://codereview.chromium.org/1355013002/diff/40001/PerformanceTests/DOM/textarea-dom.html#newcode19 PerformanceTests/DOM/textarea-dom.html:19: nodes.push(document.createTextNode('A quick brown fox jumps over the ' + ...
5 years, 2 months ago (2015-09-25 08:51:35 UTC) #21
tkent
lgtm https://codereview.chromium.org/1355013002/diff/60001/third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp File third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/1355013002/diff/60001/third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp#newcode641 third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp:641: cache->handleTextFormControlChanged(this); It's safe to change the order of ...
5 years, 2 months ago (2015-09-25 09:03:36 UTC) #22
sof
https://codereview.chromium.org/1355013002/diff/60001/third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp File third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/1355013002/diff/60001/third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp#newcode636 third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp:636: if (!textIsChanged && !innerEditor->hasChildren()) The RHS negation of (&&) ...
5 years, 2 months ago (2015-09-25 09:34:05 UTC) #23
haraken
LGTM Elliott: If you think that this is just gaming micro-benchmarks and shouldn't be landed, ...
5 years, 2 months ago (2015-09-25 09:37:47 UTC) #24
esprehn
lgtm, this seems like a general purpose optimization where we reuse the Text node directly ...
5 years, 2 months ago (2015-09-25 18:34:37 UTC) #25
sof
https://codereview.chromium.org/1355013002/diff/80001/third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp File third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/1355013002/diff/80001/third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp#newcode641 third_party/WebKit/Source/core/html/HTMLTextFormControlElement.cpp:641: // requiring less allocations. Generally helpful, but avoids dead ...
5 years, 2 months ago (2015-09-26 06:20:23 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1355013002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1355013002/100001
5 years, 2 months ago (2015-09-26 06:20:38 UTC) #29
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 2 months ago (2015-09-26 07:35:18 UTC) #30
commit-bot: I haz the power
5 years, 2 months ago (2015-09-26 07:36:14 UTC) #31
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/175ce93e9e4dd69a6191d28f7c21a63f2541f006
Cr-Commit-Position: refs/heads/master@{#350989}

Powered by Google App Engine
This is Rietveld 408576698