Bug 91335

Created attachment 152450 [details] Patch

This contains my patch for the bug 91334 as well. The patch becomes smaller once that's in.

Comment on attachment 152450 [details] Patch Attachment 152450 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13242627 New failing tests: jquery/manipulation.html

Created attachment 152453 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 152450 [details] Patch Attachment 152450 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13243627 New failing tests: jquery/manipulation.html

Created attachment 152455 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 152450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152450&action=review > Source/WebCore/html/HTMLCollection.cpp:302 > + if (overridesItemAfter()) > + return static_cast<const HTMLCollection*>(this)->virtualItemAfter(offsetInArray, toElement(previous)); > + ASSERT(!offsetInArray); > + return itemBeforeOrAfter<true>(previous); Have you seen this actually matter in benchmarks or profiles? It's a lot of complexity to add if we're not sure this matters (instead of making itemAfter always be virtual).

Comment on attachment 152450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152450&action=review >> Source/WebCore/html/HTMLCollection.cpp:302 >> + return itemBeforeOrAfter<true>(previous); > > Have you seen this actually matter in benchmarks or profiles? It's a lot of complexity to add if we're not sure this matters (instead of making itemAfter always be virtual). Making itemAfter always virtual will actually add more complexity and make the code more fragile because: 1. itemAfter needs to take & return Node* now since childNodes can contain non-element nodes; however, some of subclasses that overrides itemAfter uses itemAfter themselves so we'll need to add a lot of toElement calls. 2. Before this patch, itemAfter was always virtual. However, subclasses had to tell HTMLCollection whether it supports itemBefore (only HTMLCollection itself provides one) or not. And subclasses passed in a wrong value, it still worked when HTMLCollection was traversed forwards but hit assertions when traversed backwards. After this patch, neither forwards nor backwards traversal works if it didn't give HTMLCollection the right value.

Comment on attachment 152450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152450&action=review >>> Source/WebCore/html/HTMLCollection.cpp:302 >>> + return itemBeforeOrAfter<true>(previous); >> >> Have you seen this actually matter in benchmarks or profiles? It's a lot of complexity to add if we're not sure this matters (instead of making itemAfter always be virtual). > > Making itemAfter always virtual will actually add more complexity and make the code more fragile because: > > 1. itemAfter needs to take & return Node* now since childNodes can contain non-element nodes; however, some of subclasses that overrides itemAfter uses itemAfter themselves so we'll need to add a lot of toElement calls. > > 2. Before this patch, itemAfter was always virtual. However, subclasses had to tell HTMLCollection whether it supports itemBefore (only HTMLCollection itself provides one) or not. And subclasses passed in a wrong value, it still worked when HTMLCollection was traversed forwards but hit assertions when traversed backwards. After this patch, neither forwards nor backwards traversal works if it didn't give HTMLCollection the right value. I see. Your ChangeLog description says that this is for performance. I think the name is confusing though. How about the following changes? 1. s/virtualItemAfter/elementAfter/ 2. s/overridesItemAfter/isListOfElements/ I think that would make most of the code make more sense to me. It makes HTMLCollection::item a tad bit more confusing, but I think it's worth the clarity elsewhere in the code. Or are there HTMLCollections that are lists of Elements and don't override itemAfter?

Unfortunately there are HTMLCollections that contain only elements and don't use elements array but still override itemAfter. Maybe itemElementAfter and ProvideItemElementAfter?

Created attachment 152474 [details] Patch

I can't reproduce jquery/manipulation.html failure on mac port or chromium mac port.

Comment on attachment 152474 [details] Patch Attachment 152474 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13233869 New failing tests: jquery/manipulation.html

Created attachment 152483 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 152474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152474&action=review > Source/WebCore/html/HTMLCollection.cpp:370 > + if (isLengthCacheValid() && !overridesItemAfter() && isLastItemCloserThanLastOrCachedItem(offset)) { Is the plan for the rest of the collections to implement itemBefore? If so, I think that would make this code make a lot more sense to me. As it is, overridesItemAfter is serving multiple purposes and it makes the code very convoluted.

(In reply to comment #10) > Unfortunately there are HTMLCollections that contain only elements and don't use elements array but still override itemAfter. > > Maybe itemElementAfter and ProvideItemElementAfter? I'd prefer elementAfter and ProvidesElementAfter. Can you put elementAfter as a virtual on DynamicNodeListCacheBase and have DynamicNodeListCacheBase::itemAfter be something like: if (Element* element = elementAfter(...)) return element; ASSERT(!offsetInArray); return itemBeforeOrAfter<true>(previous); Then you don't need ProvidesElementAfter. I understand it's adding a virtual function call, but I think we should try it and see if the performance is a problem. I think this patch as is makes the code very confusing.

(In reply to comment #16) > (In reply to comment #10) > > Unfortunately there are HTMLCollections that contain only elements and don't use elements array but still override itemAfter. > > > > Maybe itemElementAfter and ProvideItemElementAfter? > > I'd prefer elementAfter and ProvidesElementAfter. That's very cumbersome because HTMLPropertiesCollection can contain multiple items inside each element in the elementsArray. > Can you put elementAfter as a virtual on DynamicNodeListCacheBase and have DynamicNodeListCacheBase::itemAfter be something like: > if (Element* element = elementAfter(...)) > return element; > ASSERT(!offsetInArray); > return itemBeforeOrAfter<true>(previous); > > Then you don't need ProvidesElementAfter. I understand it's adding a virtual function call, but I think we should try it and see if the performance is a problem. No, we can't afford to do that. Furthermore, that'll add one more vtable pointer because DynamicNodeList inherits from both NodeList and DynamicNodeListCacheBase. And no, DynamicNodeListCacheBase can't inherit from NodeList because HTMLCollection is not a NodeList.

(In reply to comment #16) > > Can you put elementAfter as a virtual on DynamicNodeListCacheBase and have DynamicNodeListCacheBase::itemAfter be something like: > if (Element* element = elementAfter(...)) > return element; > ASSERT(!offsetInArray); > return itemBeforeOrAfter<true>(previous); Also, this is semantically wrong. This change would force HTMLCollections that provide elementAfter to call both elementAfter and itemBeforeOrAfter<true>(previous) when there are items left. We'll then have to either iterate DOM twice or add a special branching at the beginning of itemBeforeOrAfter, which is added complexity and unnecessary runtime cost.

I have the exact same patch on https://bugs.webkit.org/show_bug.cgi?id=91413 with a slight modification to jquery/manipulation.html to diagnose the issue. The cr-linux EWS passed. I have no idea what the heck is going on there. I'm just going to ignore cr-linux EWS for now. Adam & Eric, it seems like EWS cr-linux has been very unreliable on this particular patch.

Created attachment 152838 [details] Patch

Created attachment 153071 [details] Fixed perf. regression

Created attachment 153072 [details] childNodes perf test Fixed the perf. regression I saw on this test. In old versions of this patch, the performance regressed as much as 100%.

Attachment 153071 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/html/HTMLCollection.cpp:258: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Committed r123281: <http://trac.webkit.org/changeset/123281>

(In reply to comment #24) > Committed r123281: <http://trac.webkit.org/changeset/123281> It made 2 tests timeout on the perf bots. I filed a new bug report to fix this regression: https://bugs.webkit.org/show_bug.cgi?id=91980 Could you check what happened?

(In reply to comment #24) > Committed r123281: <http://trac.webkit.org/changeset/123281> This caused a regression: https://bugs.webkit.org/show_bug.cgi?id=92104 TL;DR table.childNodes[table.childNodes.length - 1] returns incorrect value.