Bug 33696

Summary: let's cache nodelists instead of dynamicnodelist::cache
Product: WebKit Reporter: anton muhin <antonm>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, cmarrin, commit-queue, darin, eric.carlson, eric, jens, kling, mjs, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
First take
none
Removed unnecessary diff
sam: review-
Added a layout test
none
Patch
none
Fixing GC layout test to account for new behaviour
none
Patch
none
Patch
none
supposed crash log from commit bot
none
Patch
none
Inlining hasRareData check
none
crash log from applying patch and running tests manually
none
another crash log
none
Patch
none
Addressing Darin's concerns none

anton muhin
Reported 2010-01-14 15:58:05 PST
Current caches lead to creation of new C++ node lists for DOM queries by tag, name or class. That leads to proliferation of JS wrappers for tests like Dromaeo/DOM core/DOM query which create lots of nodelists. Let's cache nodelists per tag, name or class instead.
Attachments
First take (12.73 KB, patch)
2010-01-14 16:02 PST, anton muhin 		no flags
DetailsFormatted DiffDiff
Removed unnecessary diff (12.73 KB, patch)
2010-01-15 02:12 PST, anton muhin 		sam: review-
DetailsFormatted DiffDiff
Added a layout test (14.53 KB, patch)
2010-01-18 11:34 PST, anton muhin 		no flags
DetailsFormatted DiffDiff
Patch (14.84 KB, patch)
2010-03-17 07:51 PDT, anton muhin 		no flags
DetailsFormatted DiffDiff
Fixing GC layout test to account for new behaviour (17.47 KB, patch)
2010-03-26 06:10 PDT, anton muhin 		no flags
DetailsFormatted DiffDiff
Patch (20.28 KB, patch)
2010-03-26 12:39 PDT, anton muhin 		no flags
DetailsFormatted DiffDiff
Patch (20.11 KB, patch)
2010-03-26 13:45 PDT, anton muhin 		no flags
DetailsFormatted DiffDiff
supposed crash log from commit bot (651.95 KB, text/plain)
2010-03-28 10:34 PDT, Eric Seidel (no email) 		no flags
Details
Patch (20.12 KB, patch)
2010-03-29 08:07 PDT, anton muhin 		no flags
DetailsFormatted DiffDiff
Inlining hasRareData check (20.54 KB, patch)
2010-03-29 10:49 PDT, anton muhin 		no flags
DetailsFormatted DiffDiff
crash log from applying patch and running tests manually (615.49 KB, text/plain)
2010-03-30 11:27 PDT, Eric Seidel (no email) 		no flags
Details
another crash log (8.31 KB, text/plain)
2010-04-02 13:12 PDT, Alexey Proskuryakov 		no flags
Details
Patch (25.39 KB, patch)
2010-04-27 12:23 PDT, anton muhin 		no flags
DetailsFormatted DiffDiff
Addressing Darin's concerns (26.74 KB, patch)
2010-04-28 12:12 PDT, anton muhin 		no flags
DetailsFormatted DiffDiff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.
anton muhin
Comment 1 2010-01-14 16:02:55 PST
Created attachment 46617 [details] First take
anton muhin
Comment 2 2010-01-15 02:12:18 PST
Created attachment 46659 [details] Removed unnecessary diff
anton muhin
Comment 3 2010-01-15 04:22:39 PST
(In reply to comment #0) > Current caches lead to creation of new C++ node lists for DOM queries by tag, > name or class. That leads to proliferation of JS wrappers for tests like > Dromaeo/DOM core/DOM query which create lots of nodelists. > > Let's cache nodelists per tag, name or class instead. And forgotten. FF does some kind of caching, at least at JS level this html: <html> <head> <script> function run() { var output = document.getElementById('output'); var nodeList0 = document.getElementsByTagName('p'); var nodeList1 = document.getElementsByTagName('p'); output.innerHTML += '<p>nodeList0 == nodeList1: ' + (nodeList0 == nodeList1) + '</p>'; output.innerHTML += '<p>nodeList0 === nodeList1: ' + (nodeList0 === nodeList1) + '</p>'; } </script> </head> <body onload="run()"> <div id="output"></div> </body> </html> Gives true in FF (and false for both Safari and Chromium).
anton muhin
Comment 4 2010-01-15 04:50:03 PST
(In reply to comment #3) > (In reply to comment #0) > > Current caches lead to creation of new C++ node lists for DOM queries by tag, > > name or class. That leads to proliferation of JS wrappers for tests like > > Dromaeo/DOM core/DOM query which create lots of nodelists. > > > > Let's cache nodelists per tag, name or class instead. > > And forgotten. FF does some kind of caching, at least at JS level this html: > > <html> > <head> > <script> > function run() { > var output = document.getElementById('output'); > var nodeList0 = document.getElementsByTagName('p'); > var nodeList1 = document.getElementsByTagName('p'); > output.innerHTML += '<p>nodeList0 == nodeList1: ' + (nodeList0 == > nodeList1) + '</p>'; > output.innerHTML += '<p>nodeList0 === nodeList1: ' + (nodeList0 === > nodeList1) + '</p>'; > } > </script> > </head> > <body onload="run()"> > <div id="output"></div> > </body> > </html> > > Gives true in FF (and false for both Safari and Chromium). Minor correction: tag to lookup in run function should be div instead of p.
Darin Adler
Comment 5 2010-01-15 12:08:13 PST
Comment on attachment 46659 [details] Removed unnecessary diff This isn’t just a performance optimization. It’s a behavior change. Caching the node list object itself rather than just caching the underlying data means that custom JavaScript properties attached by one client will be seen by the other client. The client can tell when the cache is flushed by when the property does or does not go away. I don’t know if this is desirable, but I do know that we need test cases to cover it. Please do not land this patch until we explore this issue.
anton muhin
Comment 6 2010-01-15 12:26:22 PST
(In reply to comment #5) > (From update of attachment 46659 [details]) > This isn’t just a performance optimization. It’s a behavior change. Caching the > node list object itself rather than just caching the underlying data means that > custom JavaScript properties attached by one client will be seen by the other > client. The client can tell when the cache is flushed by when the property does > or does not go away. I don’t know if this is desirable, but I do know that we > need test cases to cover it. > > Please do not land this patch until we explore this issue. Definitely I won't land it until/if I get r+. Yes, I understand that's behavior change. That's exactly the reason why I wanted to see how it works in FF. In my understanding w3.org doesn't quite specify if it should be different objects or the same object, but I am biased here.
Darin Adler
Comment 7 2010-01-16 11:02:02 PST
I’m glad you did that one test case for Firefox. Can we try to define what level of caching is done?
Sam Weinig
Comment 8 2010-01-16 20:44:02 PST
Comment on attachment 46659 [details] Removed unnecessary diff r- since this needs tests due to behavior change.
anton muhin
Comment 9 2010-01-18 08:15:52 PST
(In reply to comment #7) > I’m glad you did that one test case for Firefox. Can we try to define what > level of caching is done? If I am reading this http://mxr.mozilla.org/seamonkey/source/content/base/src/nsContentList.cpp#209 right and it's right place to read, they do caching at the pretty low level (but I have no experience w/ FF codebase).
anton muhin
Comment 10 2010-01-18 08:17:14 PST
(In reply to comment #8) > (From update of attachment 46659 [details]) > r- since this needs tests due to behavior change. Sam, if consensus is that's the way to go, I'd gladly turn HTML snippet above into proper layout test. Is that what you're asking for?
Sam Weinig
Comment 11 2010-01-18 10:10:04 PST
(In reply to comment #10) > (In reply to comment #8) > > (From update of attachment 46659 [details] [details]) > > r- since this needs tests due to behavior change. > > Sam, if consensus is that's the way to go, I'd gladly turn HTML snippet above > into proper layout test. Is that what you're asking for? Yes, but you also need to test NameNodeList and ClassNodeList.
anton muhin
Comment 12 2010-01-18 11:34:50 PST
Created attachment 46836 [details] Added a layout test
anton muhin
Comment 13 2010-01-18 11:36:58 PST
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #8) > > > (From update of attachment 46659 [details] [details] [details]) > > > r- since this needs tests due to behavior change. > > > > Sam, if consensus is that's the way to go, I'd gladly turn HTML snippet above > > into proper layout test. Is that what you're asking for? > > Yes, but you also need to test NameNodeList and ClassNodeList. Layout test covering all three methods added. Thanks a lot, Sam, I would have forgotten about name and class name cases.
anton muhin
Comment 14 2010-01-19 11:18:48 PST
Guys, so what do you think about this change?
anton muhin
Comment 15 2010-01-19 11:27:59 PST
(In reply to comment #14) > Guys, so what do you think about this change? As per David suggestion. It gives pretty notable boost for Chromium in DOM core benchmark (~20%). I don't have numbers for Safari, but it should get some boost as well.
anton muhin
Comment 16 2010-01-19 11:54:51 PST
(In reply to comment #15) > (In reply to comment #14) > > Guys, so what do you think about this change? > > As per David suggestion. It gives pretty notable boost for Chromium in DOM > core benchmark (~20%). I don't have numbers for Safari, but it should get some > boost as well. And roughly the same speedup for Safari: http://dromaeo.com/?id=86080,86081 (disclaimer: both builds are only ran once, so numbers may vary, but I don't know how stable Safari runs are)
Maciej Stachowiak
Comment 17 2010-01-21 00:05:36 PST
Comment on attachment 46836 [details] Added a layout test I like the performance boost from this patch, but I am pretty sure this patch violates the DOM Level 3 specification. Here is the spec for getElementsByTagName: http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-A6C9094 Notice that it says the return value is "A new NodeList object containing all the matched Elements". So it's required that every call needs to return a new object. getElementsByTagNameNS has the same requirement: http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-getElBTNNS getElementByName and getElementsByClassName are specified by HTML5, and it does not explicitly say one way or the other whether every call to these methods must return a new object: http://dev.w3.org/html5/spec/Overview.html#dom-document-getelementsbyname http://dev.w3.org/html5/spec/Overview.html#dom-document-getelementsbyclassname But I am not sure if the difference from the DOM Level 3 Core methods is intentional; it seems possible this was just an oversight and HTML5 is not yet final. I think the benefit of caching is greater than the compatibility risk, but I would much prefer that we get the specs to explicitly allow caching. The DOM Core spec is owned by the W3C Web Apps Working Group and HTML5 is being jointly developed by the W#C HTML Working Group and the WHATWG. I suggest contacting those organizations first. Since at least part of this is explicitly disallowed by the most recent relevant spec, r-. However, I would love to see this change go in if we can get the standards aligned. I would not want to land the change if it violates standards, though, because then we will face accusations of "cheating" by violating standards to win a benchmark.
Maciej Stachowiak
Comment 18 2010-01-21 00:24:50 PST
I asked Ian Hickson (editor of HTML5) about this, and he says it's an oversight that getElementsByName and getElementsByClassName don't explicitly require a new NodeList every time. He asked me to file a bug to remind him to change it: http://www.w3.org/Bugs/Public/show_bug.cgi?id=8792 I still think the caching is good in principle, but we'll probably have to talk to the Web Apps WG and get it fixed in DOM Core first.
Maciej Stachowiak
Comment 19 2010-01-21 00:27:33 PST
I wonder how much of the win is from sharing the C++-level NodeLists and how much from sharing the wrappers. We could definitely share the C++-level objects without violating the spec, as long as we refrain from caching the wrappers. It might be worth doing that first, so we can better determine if we have good reason to ask for a spec change.
Maciej Stachowiak
Comment 21 2010-01-21 02:06:26 PST
Mozilla has been returning the same NodeList from getElementsByTagName for a long time: https://bugzilla.mozilla.org/show_bug.cgi?id=140758 It's not clear if they know this technically violates the DOM Core spec, but it does seem to imply the behavior is safe for Web compatibility.
anton muhin
Comment 22 2010-01-21 05:47:35 PST
(In reply to comment #21) > Mozilla has been returning the same NodeList from getElementsByTagName for a > long time: > https://bugzilla.mozilla.org/show_bug.cgi?id=140758 > > It's not clear if they know this technically violates the DOM Core spec, but it > does seem to imply the behavior is safe for Web compatibility. Yep, I verified that Mozilla caches node lists as well. And regarding standard. There is funny inconsistency. If you take a look at Document wording it says new, but for Element wording it doesn't say new for getElementsByTagName, but does for getElementsByTagNameNS. Answering another question: gain is due to reduced pressure on GC, so producing new JS wrappers would most probably kill the optimization. And Mozilla bug you spotted says roughly the same. So the conclusion is we are waiting for changed HTML5 spec? If yes, any ETA? And thanks a lot for very interesting investigation of the question.
Maciej Stachowiak
Comment 23 2010-01-22 01:50:57 PST
(In reply to comment #22) > (In reply to comment #21) > > Mozilla has been returning the same NodeList from getElementsByTagName for a > > long time: > > https://bugzilla.mozilla.org/show_bug.cgi?id=140758 > > > > It's not clear if they know this technically violates the DOM Core spec, but it > > does seem to imply the behavior is safe for Web compatibility. > > Yep, I verified that Mozilla caches node lists as well. > > And regarding standard. There is funny inconsistency. If you take a look at > Document wording it says new, but for Element wording it doesn't say new for > getElementsByTagName, but does for getElementsByTagNameNS. > > Answering another question: gain is due to reduced pressure on GC, so producing > new JS wrappers would most probably kill the optimization. And Mozilla bug you > spotted says roughly the same. > > So the conclusion is we are waiting for changed HTML5 spec? > > If yes, any ETA? > > And thanks a lot for very interesting investigation of the question. I think we should suggest a change or erratum to the DOM Level 3 Core spec. It seems like even if returning a new object was intended, it's not what implementations do. I suggest mailing public-webapps@w3.org to start a discussion. Or I can do it if you'd rather not deal with the standards groups.
anton muhin
Comment 24 2010-01-22 05:12:54 PST
(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > Mozilla has been returning the same NodeList from getElementsByTagName for a > > > long time: > > > https://bugzilla.mozilla.org/show_bug.cgi?id=140758 > > > > > > It's not clear if they know this technically violates the DOM Core spec, but it > > > does seem to imply the behavior is safe for Web compatibility. > > > > Yep, I verified that Mozilla caches node lists as well. > > > > And regarding standard. There is funny inconsistency. If you take a look at > > Document wording it says new, but for Element wording it doesn't say new for > > getElementsByTagName, but does for getElementsByTagNameNS. > > > > Answering another question: gain is due to reduced pressure on GC, so producing > > new JS wrappers would most probably kill the optimization. And Mozilla bug you > > spotted says roughly the same. > > > > So the conclusion is we are waiting for changed HTML5 spec? > > > > If yes, any ETA? > > > > And thanks a lot for very interesting investigation of the question. > > I think we should suggest a change or erratum to the DOM Level 3 Core spec. It > seems like even if returning a new object was intended, it's not what > implementations do. I suggest mailing public-webapps@w3.org to start a > discussion. Or I can do it if you'd rather not deal with the standards groups. Mac(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > Mozilla has been returning the same NodeList from getElementsByTagName for a > > > long time: > > > https://bugzilla.mozilla.org/show_bug.cgi?id=140758 > > > > > > It's not clear if they know this technically violates the DOM Core spec, but it > > > does seem to imply the behavior is safe for Web compatibility. > > > > Yep, I verified that Mozilla caches node lists as well. > > > > And regarding standard. There is funny inconsistency. If you take a look at > > Document wording it says new, but for Element wording it doesn't say new for > > getElementsByTagName, but does for getElementsByTagNameNS. > > > > Answering another question: gain is due to reduced pressure on GC, so producing > > new JS wrappers would most probably kill the optimization. And Mozilla bug you > > spotted says roughly the same. > > > > So the conclusion is we are waiting for changed HTML5 spec? > > > > If yes, any ETA? > > > > And thanks a lot for very interesting investigation of the question. > > I think we should suggest a change or erratum to the DOM Level 3 Core spec. It > seems like even if returning a new object was intended, it's not what > implementations do. I suggest mailing public-webapps@w3.org to start a > discussion. Or I can do it if you'd rather not deal with the standards groups. I see, thanks, Maciej. I've sent an email cc'ing you. If anyone else wants to be in the loop, just let me know.
anton muhin
Comment 25 2010-02-02 01:33:35 PST
(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > (In reply to comment #21) > > > > Mozilla has been returning the same NodeList from getElementsByTagName for a > > > > long time: > > > > https://bugzilla.mozilla.org/show_bug.cgi?id=140758 > > > > > > > > It's not clear if they know this technically violates the DOM Core spec, but it > > > > does seem to imply the behavior is safe for Web compatibility. > > > > > > Yep, I verified that Mozilla caches node lists as well. > > > > > > And regarding standard. There is funny inconsistency. If you take a look at > > > Document wording it says new, but for Element wording it doesn't say new for > > > getElementsByTagName, but does for getElementsByTagNameNS. > > > > > > Answering another question: gain is due to reduced pressure on GC, so producing > > > new JS wrappers would most probably kill the optimization. And Mozilla bug you > > > spotted says roughly the same. > > > > > > So the conclusion is we are waiting for changed HTML5 spec? > > > > > > If yes, any ETA? > > > > > > And thanks a lot for very interesting investigation of the question. > > > > I think we should suggest a change or erratum to the DOM Level 3 Core spec. It > > seems like even if returning a new object was intended, it's not what > > implementations do. I suggest mailing public-webapps@w3.org to start a > > discussion. Or I can do it if you'd rather not deal with the standards groups. > > Mac(In reply to comment #23) > > (In reply to comment #22) > > > (In reply to comment #21) > > > > Mozilla has been returning the same NodeList from getElementsByTagName for a > > > > long time: > > > > https://bugzilla.mozilla.org/show_bug.cgi?id=140758 > > > > > > > > It's not clear if they know this technically violates the DOM Core spec, but it > > > > does seem to imply the behavior is safe for Web compatibility. > > > > > > Yep, I verified that Mozilla caches node lists as well. > > > > > > And regarding standard. There is funny inconsistency. If you take a look at > > > Document wording it says new, but for Element wording it doesn't say new for > > > getElementsByTagName, but does for getElementsByTagNameNS. > > > > > > Answering another question: gain is due to reduced pressure on GC, so producing > > > new JS wrappers would most probably kill the optimization. And Mozilla bug you > > > spotted says roughly the same. > > > > > > So the conclusion is we are waiting for changed HTML5 spec? > > > > > > If yes, any ETA? > > > > > > And thanks a lot for very interesting investigation of the question. > > > > I think we should suggest a change or erratum to the DOM Level 3 Core spec. It > > seems like even if returning a new object was intended, it's not what > > implementations do. I suggest mailing public-webapps@w3.org to start a > > discussion. Or I can do it if you'd rather not deal with the standards groups. > > I see, thanks, Maciej. I've sent an email cc'ing you. If anyone else wants to > be in the loop, just let me know. I chatted with Ian this Monday. He said that nobody is actively working on this part of the spec, hence no answer. He doesn't quite like idea of caching node lists for reasons like properties assigned to result of one call would affect other calls, and would rather see other browsers not doing the caching, but says that it only could have a chance if clearly worded in the spec. On the other side "but if IE does it and it would make things faster... maybe we should just do it" So what would be resolution? Do we want to see that in WebKit?
anton muhin
Comment 26 2010-02-10 12:24:00 PST
What do you think, are there any chances of getting response from w3c? Jonas Sicking of FF is supportive, but he's the only one who replied except for Maciej.
Maciej Stachowiak
Comment 27 2010-02-12 04:06:33 PST
(In reply to comment #26) > What do you think, are there any chances of getting response from w3c? Jonas > Sicking of FF is supportive, but he's the only one who replied except for > Maciej. I'm starting to think we should just do it. Let's see if there are any conclusions from the latest round of discussion.
anton muhin
Comment 28 2010-02-26 04:12:16 PST
Sorry for pinging, but w3c thread seemed to die once again. Any resolution if we're going to commit this or discard it?
anton muhin
Comment 29 2010-03-17 07:51:12 PDT
Created attachment 50906 [details] Patch
anton muhin
Comment 30 2010-03-18 07:27:48 PDT
(In reply to comment #29) > Created an attachment (id=50906) [details] > Patch Looks like it's ultimately agreed. Could I go on and submit it?
Maciej Stachowiak
Comment 31 2010-03-25 22:54:00 PDT
It seems like the standards bodies and the browsers are moving this way so we should certainly go with this.
Oliver Hunt
Comment 32 2010-03-25 23:03:56 PDT
Comment on attachment 50906 [details] Patch r=me
anton muhin
Comment 33 2010-03-26 03:35:25 PDT
Comment on attachment 50906 [details] Patch Many thanks to everyone!
WebKit Commit Bot
Comment 34 2010-03-26 03:56:33 PDT
Comment on attachment 50906 [details] Patch Rejecting patch 50906 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12551 test cases. fast/dom/gc-9.html -> failed Exiting early after 1 failures. 5641 tests run. 87.41s total testing time 5640 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/1391011
anton muhin
Comment 35 2010-03-26 06:10:11 PDT
Created attachment 51734 [details] Fixing GC layout test to account for new behaviour
Darin Adler
Comment 36 2010-03-26 09:26:54 PDT
Comment on attachment 51734 [details] Fixing GC layout test to account for new behaviour > - [ "document.getElementsByTagName('body')" ], // NodeList > + [ "document.getElementsByTagName('body')", "allow custom skip" ], // NodeList This says that before the patch we did not allow custom properties on node lists. And after the patch we do allow custom properties on node lists. That has nothing to do with caching. Why did this patch have any effect on this? This makes no sense. I understand that the caching mean this would change from "allow custom" to "allow custom skip", but I don't see why this patch changes behavior of adding a custom property to a node list.
anton muhin
Comment 37 2010-03-26 10:02:51 PDT
(In reply to comment #36) > (From update of attachment 51734 [details]) > > - [ "document.getElementsByTagName('body')" ], // NodeList > > + [ "document.getElementsByTagName('body')", "allow custom skip" ], // NodeList > > This says that before the patch we did not allow custom properties on node > lists. And after the patch we do allow custom properties on node lists. That > has nothing to do with caching. Why did this patch have any effect on this? > This makes no sense. > > I understand that the caching mean this would change from "allow custom" to > "allow custom skip", but I don't see why this patch changes behavior of adding > a custom property to a node list. Darin, here is my understanding of the situation: LayoutTest's part: 153 function generateProperties() 154 { 155 for (var i = 0; i < objectsToTest.length; i++) { // > 156 try { 157 eval(objectsToTest[i][0] + ".myCustomProperty = 1;"); 158 } catch(e) { 159 print("NOT SUPPORTED: " + objectsToTest[i][0] + "[ " + e.message + " ]"); 160 } 161 var expectedResult = objectsToTest[i][1] ? 1 : undefined; 162 try { 163 shouldBe(objectsToTest[i][0] + ".myCustomProperty", expectedResult); 164 } catch(e) { 165 } 166 } 167 } Before each invocation of getElementsByTagName("body") would yield new object and thus assignment at line 157 wouldn't be visible at lline 163 (as we reevaluate data afresh). Now nodelists are cached and thus properties assignment as visible. I am more concerned why after GC it disappears, but suspect that JSC bindings just discard cached nodelist due to some kind of weak references. Does that make sense?
Darin Adler
Comment 38 2010-03-26 10:11:09 PDT
(In reply to comment #37) > I am more concerned why after GC it disappears, but suspect that JSC bindings > just discard cached nodelist due to some kind of weak references. > > Does that make sense? Yes that's excellent analysis, and it points out a needed code change. Since we are going to extend the lifetime of the the node lists we need to extend the lifetime of their JavaScript wrappers too. The right place to do that is the JSNode::markChildren file in JSNodeCustom.cpp, near the top. The code should get at the NodeListsNodeData structure, walk the hash tables, and call markDOMObjectWrapper on each NodeList*.
anton muhin
Comment 39 2010-03-26 10:18:20 PDT
(In reply to comment #38) > (In reply to comment #37) > > I am more concerned why after GC it disappears, but suspect that JSC bindings > > just discard cached nodelist due to some kind of weak references. > > > > Does that make sense? > > Yes that's excellent analysis, and it points out a needed code change. > > Since we are going to extend the lifetime of the the node lists we need to > extend the lifetime of their JavaScript wrappers too. The right place to do > that is the JSNode::markChildren file in JSNodeCustom.cpp, near the top. The > code should get at the NodeListsNodeData structure, walk the hash tables, and > call markDOMObjectWrapper on each NodeList*. Sure, I'd give it a try. I am only concerned if we'd like to keep this reference forever---it might be fine to GC it if it's not referenced anymore. But up to you.
Darin Adler
Comment 40 2010-03-26 10:20:47 PDT
(In reply to comment #39) > Sure, I'd give it a try. I am only concerned if we'd like to keep this > reference forever---it might be fine to GC it if it's not referenced anymore. > But up to you. The usual rule is that we don't want behavior to change based on when GC happens. So if you can add a custom property we want it to still be there later when you look for it. When the node itself goes away, the node list will go away, so that's no real problem. Once we start caching the node lists I think we need to go all the way and do the right thing with custom properties too.
anton muhin
Comment 41 2010-03-26 10:28:31 PDT
(In reply to comment #40) > (In reply to comment #39) > > Sure, I'd give it a try. I am only concerned if we'd like to keep this > > reference forever---it might be fine to GC it if it's not referenced anymore. > > But up to you. > > The usual rule is that we don't want behavior to change based on when GC > happens. So if you can add a custom property we want it to still be there later > when you look for it. > > When the node itself goes away, the node list will go away, so that's no real > problem. > > Once we start caching the node lists I think we need to go all the way and do > the right thing with custom properties too. Got it, thanks a lot. Sending a patch (hopefully) soon.
anton muhin
Comment 42 2010-03-26 12:39:56 PDT
Created attachment 51772 [details] Patch
anton muhin
Comment 43 2010-03-26 12:42:41 PDT
JS binding marking modified and gc-9 layout test changed accordingly. You might prefer not to expose hasRareData and rareData in public Node API and implement marking in the spirit of markJSListeners which would require #if USE(JSC)---just let me know how do you prefer it. If it's thing to to, I'll submit a bug against V8 bindings.
Darin Adler
Comment 44 2010-03-26 12:47:40 PDT
Comment on attachment 51772 [details] Patch > +template <class It> > +void markNodeLists(It begin, It end, MarkStack& markStack, JSGlobalData& globalData) > +{ > + for (It it = begin; it != end; ++it) > + markDOMObjectWrapper(markStack, globalData, it->second.get()); > +} I'm not a big fan of abbreviations. How about "typename IteratorType" instead of "class It"? But as long as you are using templates, I suggest putting the begin and end calls in this function too. template <class NodeListMap> void markNodeLists(const NodeListMap& map, MarkStack& markStack, JSGlobalData& globalData) { for (NodeListMap::const_iterator it = map.begin(); it != map.end; ++it) markDOMObjectWrapper(markStack, globalData, it->second.get()); } Then the call site will be cleaner. > + // NodeLists may present, if it's the case they need to be marked. Better grammar would be: "Node lists may be present. If so, they need to be marked." > + NodeListsNodeData* nodeLists = node->rareData()->nodeLists(); > + if (nodeLists) { I suggest putting the definition of the nodeLists local variable inside the if statement. > + bool hasRareData() const { return m_hasRareData; } > + NodeRareData* rareData() const; Instead of making these public, I suggest making the code to mark node lists a member of the Node class. This is the approach used in EventTarget for the markJSEventListeners function. I'm going to say r=me as-is, but I believe my suggestions would make the code significantly better.
Darin Adler
Comment 45 2010-03-26 12:48:17 PDT
(In reply to comment #43) > You might prefer not to expose hasRareData and rareData in public Node API and > implement marking in the spirit of markJSListeners which would require #if > USE(JSC)---just let me know how do you prefer it. I do. (As I mentioned in the review, which I did before I noticed this comment.)
anton muhin
Comment 46 2010-03-26 13:45:56 PDT
Created attachment 51776 [details] Patch
anton muhin
Comment 47 2010-03-26 13:46:54 PDT
Darin, I tried to address all your comments. If I missed anything, it was an oversight---please, let me know and I fix it.
Darin Adler
Comment 48 2010-03-26 14:12:55 PDT
Comment on attachment 51776 [details] Patch > +void Node::markCachedNodeLists(JSC::MarkStack& markStack, JSC::JSGlobalData& globalData) > +{ > + // NodeLists may be present. If so, they need to be marked. > + if (!hasRareData()) > + return; > + > + if (NodeListsNodeData* nodeLists = rareData()->nodeLists()) { Thanks for putting the definition inside the if statement. Now that this is in a separate function we could instead use an early return for this instead, which I think is slightly better although the definition ends up outside the if statement in that case. But the improvement is so slight that I almost don't want to mention it because I want to see this patch landed! > + markNodeLists(nodeLists->m_classNodeListCache, markStack, globalData); > + markNodeLists(nodeLists->m_nameNodeListCache, markStack, globalData); > + markNodeLists(nodeLists->m_tagNodeListCache, markStack, globalData); > + } > +} > +#if USE(JSC) > +namespace JSC { > + > +class JSGlobalData; > +class MarkStack; > + > +} > +#endif Correct formatting for this is: #if USE(JSC) namespace JSC { class JSGlobalData; class MarkStack; } #endif Even though we don't indent the entire contents of the header when it's in a namespace, we do indent inside namespaces other than the ones that enclose the entire header, like this one. See examples in almost every header file that forward declares classes from the JSC namespace. > +#include "ClassNodeList.h" > #include "DynamicNodeList.h" > #include "EventListener.h" > +#include "NameNodeList.h" > +#include "QualifiedName.h" > #include "RegisteredEventListener.h" > #include "StringHash.h" > -#include "QualifiedName.h" > +#include "TagNodeList.h" I'm kind of sad we have to add these includes rather than getting away with forward declaring more of these types, but I presume you did that because you have to. How close are we to entirely getting rid of DynamicNodeList::Caches? What uses it now? r=me as-is despite my comments above
anton muhin
Comment 49 2010-03-26 14:43:39 PDT
Comment on attachment 51776 [details] Patch Thanks a lot for review, Darin. I'll answer your comments hopefully with separate patch later---it's getting pretty late here. Let me cq+ it and hope it gets landed.
Darin Adler
Comment 50 2010-03-26 14:45:19 PDT
I had one other idea. It might be better for performance if the rare data bit check was inlined. We should do that in a follow-up patch.
WebKit Commit Bot
Comment 51 2010-03-26 21:41:37 PDT
Comment on attachment 51776 [details] Patch Rejecting patch 51776 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12557 test cases. media/video-seekable.html -> crashed Exiting early after 1 failures. 9275 tests run. 401.29s total testing time 9274 test cases (99%) succeeded 1 test case (<1%) crashed 9 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/1614008
Eric Seidel (no email)
Comment 52 2010-03-26 23:00:37 PDT
Comment on attachment 51776 [details] Patch I suspect it's a flaky test. I'll file a bug about it when I have a better internet connection.
Eric Seidel (no email)
Comment 53 2010-03-27 17:30:56 PDT
The system deleted the crash report sadly, due to too many other crashes since then.
WebKit Commit Bot
Comment 54 2010-03-27 22:09:50 PDT
Comment on attachment 51776 [details] Patch Rejecting patch 51776 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12576 test cases. media/video-seekable.html -> crashed Exiting early after 1 failures. 9294 tests run. 396.70s total testing time 9293 test cases (99%) succeeded 1 test case (<1%) crashed 9 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/1592029
WebKit Commit Bot
Comment 55 2010-03-28 06:34:23 PDT
Comment on attachment 51776 [details] Patch Rejecting patch 51776 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12576 test cases. media/video-seekable.html -> crashed Exiting early after 1 failures. 9294 tests run. 408.21s total testing time 9293 test cases (99%) succeeded 1 test case (<1%) crashed 9 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/1535051
Eric Seidel (no email)
Comment 56 2010-03-28 10:34:23 PDT
Created attachment 51864 [details] supposed crash log from commit bot According to CrashReporter, this is the crash that the commit-bot hit while trying to land your patch. I don't have the earlier crash report due to anohter bug whcih crashed invinitely and blew out my log storage limits. :(
Eric Seidel (no email)
Comment 57 2010-03-28 10:35:52 PDT
CCing some QT/media folks who might have some clue about this crash log. In the mean while I'm happy to re-queue your patch and see if it crashes a 3rd time (for a second stack trace).
Eric Seidel (no email)
Comment 58 2010-03-28 10:36:50 PDT
Comment on attachment 51776 [details] Patch re-queueing on the off chance that the affected test is just flaky and that lighting struck twice.
WebKit Commit Bot
Comment 59 2010-03-28 11:24:54 PDT
Comment on attachment 51776 [details] Patch Rejecting patch 51776 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12576 test cases. media/video-seekable.html -> crashed Exiting early after 1 failures. 9294 tests run. 397.66s total testing time 9293 test cases (99%) succeeded 1 test case (<1%) crashed 9 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/1565040
Alexey Proskuryakov
Comment 60 2010-03-28 14:05:39 PDT
Comment on attachment 51776 [details] Patch Marking r-, because this appears to corrupt memory, making a test crash.
anton muhin
Comment 61 2010-03-29 08:07:43 PDT
Created attachment 51911 [details] Patch
WebKit Review Bot
Comment 62 2010-03-29 08:13:16 PDT
Attachment 51911 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/dom/Node.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
anton muhin
Comment 63 2010-03-29 08:13:47 PDT
(In reply to comment #48) > (From update of attachment 51776 [details]) > > +void Node::markCachedNodeLists(JSC::MarkStack& markStack, JSC::JSGlobalData& globalData) > > +{ > > + // NodeLists may be present. If so, they need to be marked. > > + if (!hasRareData()) > > + return; > > + > > + if (NodeListsNodeData* nodeLists = rareData()->nodeLists()) { > > Thanks for putting the definition inside the if statement. > > Now that this is in a separate function we could instead use an early return > for this instead, which I think is slightly better although the definition ends > up outside the if statement in that case. > > But the improvement is so slight that I almost don't want to mention it because > I want to see this patch landed! Done. > > + markNodeLists(nodeLists->m_classNodeListCache, markStack, globalData); > > + markNodeLists(nodeLists->m_nameNodeListCache, markStack, globalData); > > + markNodeLists(nodeLists->m_tagNodeListCache, markStack, globalData); > > + } > > +} > > +#if USE(JSC) > > +namespace JSC { > > + > > +class JSGlobalData; > > +class MarkStack; > > + > > +} > > +#endif > > Correct formatting for this is: > > #if USE(JSC) > namespace JSC { > class JSGlobalData; > class MarkStack; > } > #endif > > Even though we don't indent the entire contents of the header when it's in a > namespace, we do indent inside namespaces other than the ones that enclose the > entire header, like this one. See examples in almost every header file that > forward declares classes from the JSC namespace. Done, but now style checking script barks at me. And I used 4 space indent instead of 2 spaces, was I wrong? > > > +#include "ClassNodeList.h" > > #include "DynamicNodeList.h" > > #include "EventListener.h" > > +#include "NameNodeList.h" > > +#include "QualifiedName.h" > > #include "RegisteredEventListener.h" > > #include "StringHash.h" > > -#include "QualifiedName.h" > > +#include "TagNodeList.h" > > I'm kind of sad we have to add these includes rather than getting away with > forward declaring more of these types, but I presume you did that because you > have to. Alas, PassRefPtr requires full definition. > How close are we to entirely getting rid of DynamicNodeList::Caches? What uses > it now? I hoped to do that as well, but that was necessary for child list. I'll try to kill it later (if possible, ok?)
anton muhin
Comment 64 2010-03-29 08:14:50 PDT
(In reply to comment #50) > I had one other idea. It might be better for performance if the rare data bit > check was inlined. We should do that in a follow-up patch. Is not it inlined already (if you mean hasRareData())?
anton muhin
Comment 65 2010-03-29 08:17:33 PDT
(In reply to comment #60) > (From update of attachment 51776 [details]) > Marking r-, because this appears to corrupt memory, making a test crash. Sure, Alexey, thanks. So to summarize: it looks like QT-only problem? And I wonder if it can be reproduced if I alter LayoutTest slightly---instead of getting nodelists on each call, one could cache them by tag name in JavaScript itself. If it make QT fail, I'd rather blame QT than this patch. Should I try to do it? Going to try to build QT variant, but if someone could help me to debug that out, the help would be most appreciated. All I need is some access to a box where QT flavour could be built + gdb.
Alexey Proskuryakov
Comment 66 2010-03-29 08:45:52 PDT
I believe it crashes on Mac. QT is QuickTime, Qt is Nokia.
Darin Adler
Comment 67 2010-03-29 09:01:51 PDT
(In reply to comment #63) > Done, but now style checking script barks at me. Bug in the script. > And I used 4 space indent > instead of 2 spaces, was I wrong? No.
Darin Adler
Comment 68 2010-03-29 09:03:57 PDT
(In reply to comment #64) > (In reply to comment #50) > > I had one other idea. It might be better for performance if the rare data bit > > check was inlined. We should do that in a follow-up patch. > > Is not it inlined already (if you mean hasRareData())? In JSNode::markChildren, we make a function call to markCachedNodeLists. I was looking for a code change to make sure it markChildren makes no function call in the common case where hasRareData() is false.
Adam Barth
Comment 69 2010-03-29 09:06:06 PDT
> > Done, but now style checking script barks at me. > > Bug in the script. Is it? +namespace JSC { + + class JSGlobalData; + class MarkStack; + +} versus Rule #3 <http://webkit.org/coding/coding-style.html>?
Darin Adler
Comment 70 2010-03-29 09:07:38 PDT
(In reply to comment #69) > > > Done, but now style checking script barks at me. > > > > Bug in the script. > > Is it? > > +namespace JSC { > + > + class JSGlobalData; > + class MarkStack; > + > +} > > versus Rule #3 <http://webkit.org/coding/coding-style.html>? There's nothing specific in the coding style document to make it clear that indenting is desirable in forward declarations as opposed to the actual code defined in a header. But there should be.
Adam Barth
Comment 71 2010-03-29 09:16:48 PDT
> But there should be. https://bugs.webkit.org/show_bug.cgi?id=36760
anton muhin
Comment 72 2010-03-29 09:31:04 PDT
(In reply to comment #66) > I believe it crashes on Mac. QT is QuickTime, Qt is Nokia. Aha, I thought it qt port, sorry. It still doesn't crash on my MacBook, I wonder if we have different set of codecs. Eric, is there a way to find it out?
anton muhin
Comment 73 2010-03-29 09:32:52 PDT
(In reply to comment #68) > (In reply to comment #64) > > (In reply to comment #50) > > > I had one other idea. It might be better for performance if the rare data bit > > > check was inlined. We should do that in a follow-up patch. > > > > Is not it inlined already (if you mean hasRareData())? > > In JSNode::markChildren, we make a function call to markCachedNodeLists. I was > looking for a code change to make sure it markChildren makes no function call > in the common case where hasRareData() is false. I see, thanks. Let me give it a try.
anton muhin
Comment 74 2010-03-29 10:49:28 PDT
Created attachment 51930 [details] Inlining hasRareData check
anton muhin
Comment 75 2010-03-29 10:51:21 PDT
(In reply to comment #73) > (In reply to comment #68) > > (In reply to comment #64) > > > (In reply to comment #50) > > > > I had one other idea. It might be better for performance if the rare data bit > > > > check was inlined. We should do that in a follow-up patch. > > > > > > Is not it inlined already (if you mean hasRareData())? > > > > In JSNode::markChildren, we make a function call to markCachedNodeLists. I was > > looking for a code change to make sure it markChildren makes no function call > > in the common case where hasRareData() is false. > > I see, thanks. Let me give it a try. Check inlined. I experimented with nodeLists fetching but that requires NodeRareData.h include which conflicts with export headers.
WebKit Review Bot
Comment 76 2010-03-29 10:54:16 PDT
Attachment 51930 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/dom/Node.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Eric Seidel (no email)
Comment 77 2010-03-30 11:27:37 PDT
Created attachment 52059 [details] crash log from applying patch and running tests manually running media/video-reverse-play-duration.html -> crashed stderr output from test: DumpRenderTree(36023,0xa01aa720) malloc: *** mmap(size=774144) failed (error code=12) *** error: can't allocate region *** set a breakpoint in malloc_error_break to debug Maybe there is a big leak in this patch? The stdout from the test was: Invalid memory access of location 0x278 eip=0x22d1d406
Eric Seidel (no email)
Comment 78 2010-03-30 11:42:24 PDT
It looks from the latest failure log like this change is causing memory a malloc deep inside QuickTime to fail. Likely due to memory exhaustion although possibly from some other form of corruption. I would expect it should be easy to reproduce on any other hardware. I'm currently doing a --leaks run of run-webkit-tests on the commit-bot hardware in hopes that produces interesting results for you.
Eric Seidel (no email)
Comment 79 2010-03-30 12:06:03 PDT
run-webkit-tests media produces a crash (video-seeking.html) on the commit bot. It's a different test than when you run all of the tests together (video-reverse-play-duration.html). Is it possible that this is now causing some extra <audio>/<video> element caching which is upsetting quicktime? Also, looking at the ChangeLog it seems rather sparse in this patch. Were I reviewing the change, I would want more information of what was actually going on. I'm testing on my Mac Book Pro right now to see if I'm able to reproduce the crash there too.
Eric Seidel (no email)
Comment 80 2010-03-30 13:31:58 PDT
run-webkit-tests media on my mac book pro did not produce a crash like on the commit bot. However it did produce one failure: --- /tmp/layout-test-results/media/adopt-node-crash-expected.txt 2010-03-30 13:29:02.000000000 -0700 +++ /tmp/layout-test-results/media/adopt-node-crash-actual.txt 2010-03-30 13:29:02.000000000 -0700 @@ -1 +1,6 @@ -SUCCESS +CONSOLE MESSAGE: line 16: TypeError: Result of expression 'document.getElementById('console')' [null] is not an object. +FAIL: Timed out waiting for notifyDone to be called + +Tests that we don't crash when moving a video element to a new document. + +
Eric Seidel (no email)
Comment 81 2010-03-30 13:34:39 PDT
All tests passed on my macbook pro when I ran: run-webkit-tests inspector java loader media I'm not yet sure what's wrong here.
Eric Seidel (no email)
Comment 82 2010-03-30 13:39:18 PDT
subsequent "run-webkit-tests media" runs have not shown any failures, so perhaps that was just a fluke. I'm attempting to reproduce the crash on another Mac Pro.
Eric Seidel (no email)
Comment 83 2010-04-01 17:25:59 PDT
I failed to repro the crash on my other mac pro.
Eric Seidel (no email)
Comment 84 2010-04-02 10:39:27 PDT
Comment on attachment 51930 [details] Inlining hasRareData check We've failed to reproduce this on several other machines. I'm going to assume that the commit-queue was just confused and try land this patch via the cq one more time.
Alexey Proskuryakov
Comment 85 2010-04-02 11:49:43 PDT
Comment on attachment 51930 [details] Inlining hasRareData check Did anyone at least try to run tests with GuardMalloc? The insistence on getting an apparent memory corruption bug in the tree amazes me.
anton muhin
Comment 86 2010-04-02 11:52:32 PDT
(In reply to comment #85) > (From update of attachment 51930 [details]) > Did anyone at least try to run tests with GuardMalloc? The insistence on > getting an apparent memory corruption bug in the tree amazes me. Yes, with both -g and -l
Eric Seidel (no email)
Comment 87 2010-04-02 12:00:28 PDT
Comment on attachment 51930 [details] Inlining hasRareData check I'm not sure I'd call it insistence. :) I tried on 3 other machine and failed to reproduce the failure, so I stopped trusting the results from the commit-queue. Anton tried on several machines himself. Marking cq- is actually worse than marking r- as it would encourage someone to land this manually, where as the cq is the only machine which has seen this failure. I'll clear the flags and you and Anton can hash it out.
Alexey Proskuryakov
Comment 88 2010-04-02 12:46:17 PDT
Could someone recap what testing was performed here? Specifically, comment 77 has made me believe that Eric did get a mysterious crash with this patch when testing manually. It's great that GuardMalloc tests were performed, but this didn't seem to be mentioned before. Can you try running DOM fuzzers (like iexploder, mangleme, jsfunfuzz or one from bug 29692) with this patch applied? In the meanwhile, I'll try to run regression tests with this patch applied, too. It's certainly painful, and the road forward is not obvious, but it's better to suffer now than to introduce a memory corruption bug for bad guys to exploit.
Eric Seidel (no email)
Comment 89 2010-04-02 12:51:47 PDT
My testing: run-webkit-tests (full and just media) on 8-core MacPro commit-bot : crashed. run-webkit-tests (full and just media) on 4-core MacPro: no crash run-webkit-tests media on work Intel Core Duo 2 MBP: no crash run-webkit-tests media on personal Intel Core Duo 2 MBP: no crash
Alexey Proskuryakov
Comment 90 2010-04-02 13:12:28 PDT
Created attachment 52442 [details] another crash log I'm also getting random crashes (Mac Pro, Mac OS X 10.6.3, so it was 64 bit). The attached crash log looks directly related to this change.
Eric Seidel (no email)
Comment 91 2010-04-02 13:15:18 PDT
YAY! The commit-bot is not crazy. Thank you Alexey. Ball is in your court Anton. :)
anton muhin
Comment 92 2010-04-05 07:40:14 PDT
(In reply to comment #90) > Created an attachment (id=52442) [details] > another crash log > > I'm also getting random crashes (Mac Pro, Mac OS X 10.6.3, so it was 64 bit). > The attached crash log looks directly related to this change. Thanks a lot, Alexey and Eric. Your crash log, Alexey, looks very very useful---thanks again. While I'm digging into it, could you, Alexey, provide more details about hardware you ran it, e.g. how many cores, ram, etc. it has? If it's more or less regular Mac Pro, then I'd be able to run on pretty close config in my office. Thanks again, guys!
Alexey Proskuryakov
Comment 93 2010-04-05 08:42:03 PDT
I ran the tests on an 8 core Mac Pro.
anton muhin
Comment 94 2010-04-12 12:34:33 PDT
Sorry for long delayed. I suspect that my patch might be not to blame, but there is some problem with 64 port of WebKit. When I load LayoutTests/media/video-seekable.html directly into freshly build Safari, I've got the following warning: Starting Safari with DYLD_FRAMEWORK_PATH set to point to built WebKit in /Users/chrome-user/WebKit-A/WebKitBuild/Release. FlateDecode: decoding error: incorrect data check. Is it safe? I don't see anything like this for less powerful platform and excluding media tests make LayoutTests pass. I do understand it's not a proof, but rather would like to resort to you---would do you think? Might this warning be a real problem (many stack traces shows this QT stuff)? Is there a bug for that (I failed to find one)?
Alexey Proskuryakov
Comment 95 2010-04-12 18:18:26 PDT
This is unlikely to be related. The crashes I was getting were on fast/dom tests.
anton muhin
Comment 96 2010-04-13 09:11:52 PDT
(In reply to comment #95) > This is unlikely to be related. The crashes I was getting were on fast/dom > tests. I am not so sure, as corrupted memory could manifest itself long after the test. On my box it's not media tests that fail (at least not always), still if I exclude media tests from run, everything is fine. Anyway, this warning comes not from WebKit proper, but from some system calls, correct? Is there a way to debug those?
Alexey Proskuryakov
Comment 97 2010-04-13 09:21:14 PDT
Since fast/dom goes alphabetically before media, DOM tests should run earlier than media ones.
anton muhin
Comment 98 2010-04-22 05:43:22 PDT
Update. With https://bugs.webkit.org/show_bug.cgi?id=37722 I could pass all the tests. I need this patch to solve memory leak issue that might be a reason for random crashes we observe---with the current patch NodeList refs Node and Node now refs NodeLists (via caches). I've got a preliminary patch which breaks this circular dep.
anton muhin
Comment 99 2010-04-27 12:23:51 PDT
Created attachment 54443 [details] Patch
anton muhin
Comment 100 2010-04-27 12:25:43 PDT
(In reply to comment #99) > Created an attachment (id=54443) [details] > Patch New patch solves the issue with circular references between nodes and node lists. It passes layout tests on two platforms I tested: MBP and 8 core MacPro.
WebKit Review Bot
Comment 101 2010-04-27 12:28:42 PDT
Attachment 54443 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/dom/Node.h:38: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 102 2010-04-27 12:40:22 PDT
Comment on attachment 54443 [details] Patch > + , m_classNamesOriginal(classNames) Instead of adding this and making every ClassNodeList bigger, you should add an originalString() function to the SpaceSplitString class, which already keeps a copy of the original string. (If you kept this, then you'd want to give it a better name.) > + ASSERT(list->hasOwnCaches()); > + UNUSED_PARAM(list); The ASSERT_UNUSED macro should be used in cases like this. > + ASSERT(list == data->m_classNodeListCache.get(className)); The ASSERT_UNUSED macro should be used in cases like this. > + ASSERT(list->hasOwnCaches()); > + UNUSED_PARAM(list); The ASSERT_UNUSED macro should be used in cases like this. > + ASSERT(list == data->m_nameNodeListCache.get(nodeName)); The ASSERT_UNUSED macro should be used in cases like this. > + ASSERT(list->hasOwnCaches()); > + UNUSED_PARAM(list); The ASSERT_UNUSED macro should be used in cases like this. > + ASSERT(list == data->m_tagNodeListCache.get(name)); The ASSERT_UNUSED macro should be used in cases like this. > + PassRefPtr<TagNodeList> list = TagNodeList::create(this, namespaceURI.isEmpty() ? nullAtom : namespaceURI, localNameAtom); Local variables should have a type of RefPtr, not PassRefPtr. See <http://webkit.org/coding/RefPtr.html>. > + PassRefPtr<NameNodeList> list = NameNodeList::create(this, elementName); Local variables should have a type of RefPtr, not PassRefPtr. See <http://webkit.org/coding/RefPtr.html>. > + m_rootNode->removeCachedTagNodeList(this, QualifiedName(nullAtom, m_localName, m_namespaceURI)); We might want to store this as a QualifiedName in the first place.
anton muhin
Comment 103 2010-04-28 06:30:45 PDT
(In reply to comment #102) > (From update of attachment 54443 [details]) > > + , m_classNamesOriginal(classNames) > > Instead of adding this and making every ClassNodeList bigger, you should add an > originalString() function to the SpaceSplitString class, which already keeps a > copy of the original string. (If you kept this, then you'd want to give it a > better name.) I've started what you proposed but noticed that SpaceSplitStringData::createVector mutates string: 49 if (m_shouldFoldCase && hasNonASCIIOrUpper(m_string)) 50 m_string = m_string.foldCase(); So for now I decided to keep a copy (renamed to m_originalClassNames), but I'll do something about it if you still prefer to fetch original string from the SpaceSplitString. After all I could probably use it as a key (just thinking) > > > + ASSERT(list->hasOwnCaches()); > > + UNUSED_PARAM(list); Thanks a lot for pointing this out, fixed. > > The ASSERT_UNUSED macro should be used in cases like this. > > > + ASSERT(list == data->m_classNodeListCache.get(className)); > > The ASSERT_UNUSED macro should be used in cases like this. > > > + ASSERT(list->hasOwnCaches()); > > + UNUSED_PARAM(list); > > The ASSERT_UNUSED macro should be used in cases like this. > > > + ASSERT(list == data->m_nameNodeListCache.get(nodeName)); > > The ASSERT_UNUSED macro should be used in cases like this. > > > + ASSERT(list->hasOwnCaches()); > > + UNUSED_PARAM(list); > > The ASSERT_UNUSED macro should be used in cases like this. > > > + ASSERT(list == data->m_tagNodeListCache.get(name)); > > The ASSERT_UNUSED macro should be used in cases like this. Ditto for all above. > > > + PassRefPtr<TagNodeList> list = TagNodeList::create(this, namespaceURI.isEmpty() ? nullAtom : namespaceURI, localNameAtom); > > Local variables should have a type of RefPtr, not PassRefPtr. See > <http://webkit.org/coding/RefPtr.html>. > > > + PassRefPtr<NameNodeList> list = NameNodeList::create(this, elementName); > > Local variables should have a type of RefPtr, not PassRefPtr. See > <http://webkit.org/coding/RefPtr.html>. Thanks a lot, fixed. > > > + m_rootNode->removeCachedTagNodeList(this, QualifiedName(nullAtom, m_localName, m_namespaceURI)); > > We might want to store this as a QualifiedName in the first place. Directly in the TagNodeList object like I currently do for ClassNodeList? I'll upload new version of the patch when it'll pass layout tests. Thanks a lot for comments, Darin.
Darin Adler
Comment 104 2010-04-28 10:53:43 PDT
(In reply to comment #103) > (In reply to comment #102) > > (From update of attachment 54443 [details] [details]) > > > + , m_classNamesOriginal(classNames) > > > > Instead of adding this and making every ClassNodeList bigger, you should add an > > originalString() function to the SpaceSplitString class, which already keeps a > > copy of the original string. (If you kept this, then you'd want to give it a > > better name.) > > I've started what you proposed but noticed that > SpaceSplitStringData::createVector mutates string: > > 49 if (m_shouldFoldCase && hasNonASCIIOrUpper(m_string)) > 50 m_string = m_string.foldCase(); > > So for now I decided to keep a copy (renamed to m_originalClassNames), but I'll > do something about it if you still prefer to fetch original string from the > SpaceSplitString. After all I could probably use it as a key (just thinking) I was completely wrong. SpaceSplitString does *not* keep the string around after parsing it. At the end of that function it sets m_string to null.
anton muhin
Comment 105 2010-04-28 12:12:39 PDT
Created attachment 54604 [details] Addressing Darin's concerns
WebKit Review Bot
Comment 106 2010-04-28 12:17:27 PDT
Attachment 54604 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/dom/Node.h:38: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
anton muhin
Comment 107 2010-04-29 06:48:05 PDT
Just in case, I've uploaded new version which should address all Darin's concerns (see the latest attachment)
anton muhin
Comment 108 2010-04-29 08:15:57 PDT
Comment on attachment 54604 [details] Addressing Darin's concerns Thanks a lot, Daring, cq+'ing it.
WebKit Commit Bot
Comment 109 2010-04-29 11:02:58 PDT
Comment on attachment 54604 [details] Addressing Darin's concerns Clearing flags on attachment: 54604 Committed r58526: <http://trac.webkit.org/changeset/58526>
WebKit Commit Bot
Comment 110 2010-04-29 11:03:15 PDT
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 111 2010-06-11 15:21:29 PDT
*** Bug 31308 has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 112 2010-06-11 15:22:17 PDT
This fix was incomplete, see bug 9508 for a case where we still diverge from Firefox.
anton muhin
Comment 113 2010-06-15 09:37:33 PDT
(In reply to comment #112) > This fix was incomplete, see bug 9508 for a case where we still diverge from Firefox. Just FYI. Opera caches the results of querySelectorAll. querySelectorAll of Opera 10.60 seems to be faster by itself, but esp. with caching it allows very good performance in some CSS selector benchmark for Prototype JS lib. And for all WebKit-based browsers Prototype resorts to JS filtering and doesn't use querySelectorAll at all.
Alexey Proskuryakov
Comment 114 2010-11-30 12:22:43 PST
> This fix was incomplete, see bug 9508 for a case where we still diverge from Firefox. Another example: bug 17184.
Note You need to log in before you can comment on or make changes to this bug.