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

Issue 159503003: Do not cause unnecessary node lists invalidation on id/name attribute change (Closed)

Created:
6 years, 10 months ago by Inactive
Modified:
6 years, 10 months ago
CC:
blink-reviews, dglazkov+blink, sof, eae+blinkwatch, adamk+blink_chromium.org, eseidel
Visibility:
Public.

Description

Do not cause unnecessary node lists invalidation on id/name attribute change Do not cause unnecessary node lists invalidation on id/name attribute change. Invalidation now occurs only if there are node lists with a *valid* id / name cache. I see a 12% speed up on Dromaeo's dom-attr test. This patch should fix the performance regression by r166263, which made getElementsByTagName() return an HTMLCollection instead of a NodeList. According to the profiler, the performance impact is mainly due to node lists cache invalidation. This is because HTMLCollection has an id / name cache, while NodeList doesn't. This CL also improves the id / name cache API naming on HTMLCollection for clarity. R=abarth, adamk BUG=340325 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167115

Patch Set 1 #

Patch Set 2 : Invalidate id/name cache in HTMLCollection destructor #

Patch Set 3 : Handle move between documents properly #

Patch Set 4 : Code clean up #

Total comments: 6

Patch Set 5 : Rebase #

Total comments: 5

Patch Set 6 : Rebase #

Patch Set 7 : Rebase #

Patch Set 8 : Rebase #

Patch Set 9 : Rebase #

Patch Set 10 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -35 lines) Patch
M Source/core/dom/Document.h View 1 2 3 4 2 chunks +13 lines, -0 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 5 2 chunks +0 lines, -4 lines 0 comments Download
M Source/core/dom/LiveNodeList.h View 1 2 3 4 5 3 chunks +3 lines, -2 lines 0 comments Download
M Source/core/dom/LiveNodeList.cpp View 1 2 3 4 5 2 chunks +8 lines, -1 line 0 comments Download
M Source/core/dom/NodeRareData.h View 1 2 3 4 1 chunk +2 lines, -5 lines 0 comments Download
M Source/core/html/HTMLAllCollection.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/HTMLCollection.h View 1 2 3 3 chunks +26 lines, -7 lines 0 comments Download
M Source/core/html/HTMLCollection.cpp View 1 2 3 4 5 7 chunks +10 lines, -8 lines 0 comments Download
M Source/core/html/HTMLFormControlsCollection.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/html/HTMLFormControlsCollection.cpp View 1 2 3 chunks +5 lines, -5 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
Inactive
6 years, 10 months ago (2014-02-11 05:12:43 UTC) #1
Inactive
fast/dom/collection-nameditem-move-between-documents.html is failing in debug mode. I will investigate and fix, sorry about that.
6 years, 10 months ago (2014-02-11 13:59:23 UTC) #2
Inactive
On 2014/02/11 13:59:23, Chris Dumez wrote: > fast/dom/collection-nameditem-move-between-documents.html is failing in debug > mode. I ...
6 years, 10 months ago (2014-02-11 16:23:10 UTC) #3
eseidel
https://codereview.chromium.org/159503003/diff/200001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/159503003/diff/200001/Source/core/dom/Document.h#newcode1384 Source/core/dom/Document.h:1384: m_nodeListCounts[InvalidateOnIdNameAttrChange]++; This seems about the same size as just ...
6 years, 10 months ago (2014-02-12 01:55:24 UTC) #4
Inactive
https://codereview.chromium.org/159503003/diff/200001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/159503003/diff/200001/Source/core/dom/Document.h#newcode1384 Source/core/dom/Document.h:1384: m_nodeListCounts[InvalidateOnIdNameAttrChange]++; On 2014/02/12 01:55:24, eseidel wrote: > This seems ...
6 years, 10 months ago (2014-02-12 02:21:41 UTC) #5
Inactive
https://codereview.chromium.org/159503003/diff/200001/Source/core/dom/NodeRareData.h File Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/159503003/diff/200001/Source/core/dom/NodeRareData.h#newcode176 Source/core/dom/NodeRareData.h:176: if (oldDocument == newDocument) { On 2014/02/12 02:21:41, Chris ...
6 years, 10 months ago (2014-02-12 02:38:47 UTC) #6
Inactive
On 2014/02/12 02:38:47, Chris Dumez wrote: > https://codereview.chromium.org/159503003/diff/200001/Source/core/dom/NodeRareData.h > File Source/core/dom/NodeRareData.h (right): > > https://codereview.chromium.org/159503003/diff/200001/Source/core/dom/NodeRareData.h#newcode176 ...
6 years, 10 months ago (2014-02-12 14:12:02 UTC) #7
Inactive
Added some comments to clarify the reasoning behind my changes. https://codereview.chromium.org/159503003/diff/290001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (left): https://codereview.chromium.org/159503003/diff/290001/Source/core/dom/Document.cpp#oldcode3563 ...
6 years, 10 months ago (2014-02-12 18:26:07 UTC) #8
Inactive
Any more feedback on this? I'd hate to go back to using NodeLists because of ...
6 years, 10 months ago (2014-02-13 18:59:06 UTC) #9
eseidel
lgtm Sorry to be slow.
6 years, 10 months ago (2014-02-13 19:39:32 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/159503003/290001
6 years, 10 months ago (2014-02-13 19:39:43 UTC) #11
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-13 19:39:47 UTC) #12
commit-bot: I haz the power
Failed to apply patch for Source/core/dom/LiveNodeList.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
6 years, 10 months ago (2014-02-13 19:39:48 UTC) #13
Inactive
On 2014/02/13 19:39:32, eseidel wrote: > lgtm > > Sorry to be slow. Thanks, I ...
6 years, 10 months ago (2014-02-13 19:45:44 UTC) #14
Inactive
The CQ bit was checked by ch.dumez@samsung.com
6 years, 10 months ago (2014-02-13 19:55:27 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/159503003/510001
6 years, 10 months ago (2014-02-13 19:55:51 UTC) #16
commit-bot: I haz the power
6 years, 10 months ago (2014-02-13 21:19:43 UTC) #17
Message was sent while issue was closed.
Change committed as 167115

Powered by Google App Engine
This is Rietveld 408576698