Bug 31308

Summary: NodeLists generated by the same queries should be cached.
Product: WebKit Reporter: Jens Alfke <jens>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Enhancement CC: ap, jens, oliver, sam
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Shark profile of Dromaeo DOM Query test none

Jens Alfke
Reported 2009-11-10 12:47:32 PST
V8DOMWrapper caches V8 proxy objects for most native DOM objects, but it doesn't do so for NodeLists. This means that every time a JS function like getElementsByTagName is called, a new JS object has to be created for the return value. This has a pretty noticeable effect in the Dromaeo DOM Query test — I used Shark to sample it on OS X and it's spending 1/3 of its time in V8DOMWrapper::convertToV8Object, most of which is spent in instantiateV8Object. There's a lot of GC going on too. I _think_ it would be straightforward to extend V8DOMWrapper to cache NodeLists; but I'm not familiar with the details of how the caching is done.
Attachments
Shark profile of Dromaeo DOM Query test (667.51 KB, application/octet-stream)
2009-11-10 12:51 PST, Jens Alfke 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Jens Alfke
Comment 1 2009-11-10 12:51:57 PST
Created attachment 42887 [details] Shark profile of Dromaeo DOM Query test
Sam Weinig
Comment 2 2009-11-10 14:54:01 PST
This would actually change functionality as the behavior of == would change. You need to check what other browsers do before making this change.
Jens Alfke
Comment 3 2009-11-10 16:51:26 PST
Good point. I'm pretty sure that Safari caches NodeLists, because it scores twice as high on that benchmark and I think the reason why is that it's not constantly creating new objects. But this needs confirmation, of course.
Sam Weinig
Comment 4 2009-11-10 18:38:53 PST
That is easy to test. javascript:alert(document.getElementsByTagName('a') == document.getElementsByTagName('a')) I get false in Safari.
Sam Weinig
Comment 5 2009-11-10 18:44:17 PST
It does, however, return true for Firefox.
Sam Weinig
Comment 6 2009-11-10 18:48:04 PST
If we do decide to do NodeList caching, it should probably be done in the DOM, and not in the wrappers. That is to say, calling Document::getElementsByTagName('a') twice should return the same C++ NodeList object.
Oliver Hunt
Comment 7 2010-03-04 01:56:04 PST
Retitling this bug as the caching is in the DOM implementation so (one would hope) it will be entirely JS engine independent. I believe maciej bought this up on the whatwg list recently, i'm not sure what the outcome was
Alexey Proskuryakov
Comment 8 2010-06-11 15:21:29 PDT
This specific case was fixed in bug 33696. *** This bug has been marked as a duplicate of bug 33696 ***
Note You need to log in before you can comment on or make changes to this bug.