Summary: | [Performance] Optimize sequentially accessed childNodes[] | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||||
Component: | DOM | Assignee: | Kentaro Hara <haraken> | ||||||||
Status: | RESOLVED WONTFIX | ||||||||||
Severity: | Normal | CC: | abarth, ahmad.saleem792, ap, arv, bfulgham, dglazkov, eric, rniwa, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Kentaro Hara
2012-03-17 08:32:48 PDT
Created attachment 132460 [details]
Performance test
Created attachment 132461 [details]
Patch
Comment on attachment 132461 [details] Patch Attachment 132461 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11964894 Comment on attachment 132461 [details] Patch Attachment 132461 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11966841 New failing tests: jquery/core.html jquery/manipulation.html jquery/traversing.html This has some issues. You need to ensure that you don't cache null items. Created attachment 132489 [details]
Patch
(In reply to comment #5) > This has some issues. You need to ensure that you don't cache null items. Good point. Fixed. Thanks. Comment on attachment 132489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132489&action=review > In most cases childNodes[] are accessed sequentially. We can optimize the fast path for sequential accesses. Please provide evidence for this claim. > Source/WebCore/dom/ChildNodeList.h:46 > + virtual Node* item(unsigned index) const OVERRIDE > + { > + // fast path What's the benefit of putting this function implementation into header? Is it ever called directly to be inlined? > Source/WebCore/dom/ChildNodeList.h:71 > + virtual Node* itemSlow(unsigned index) const OVERRIDE; Why is this function virtual, and what does it override? Do you really need to split this into two functions? Comment on attachment 132489 [details]
Patch
I feel like this one could be restructured to be a lot clearer. Both item and itemSlow try to be fast. In both cases we only have to do previousSibling once if index === lastItemOffset - 1.
The difference is that we are doing a lot of extra work in the common cases of lastItemOffset+1 and lastItemOffset-1. By restructuring the code I think we could make this cleaner and still get a performance boost for the common cases.
Comment on attachment 132489 [details] Patch Attachment 132489 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11985488 Comment on attachment 132489 [details]
Patch
Thanks for comments. I'll update the patch tomorrow.
*** Safari 15.6 on macOS 12.5 *** sequential childNodes : 312.8ms reverse sequential childNodes : 304.4ms *** Chrome Canary 106 *** sequential childNodes : 1156.8ms reverse sequential childNodes : 1171ms *** Firefox Nightly 105 *** sequential childNodes : 856.6ms reverse sequential childNodes : 867.2ms ________ Safari is the fastest in the attached test case, I don't think this patch landed, can we work on it (via rebase) or mark it as "RESOLVED LATER"? Thanks! This is won't fix at this point. |