WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 33696
let's cache nodelists instead of dynamicnodelist::cache
https://bugs.webkit.org/show_bug.cgi?id=33696
Summary
let's cache nodelists instead of dynamicnodelist::cache
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
Details
Formatted Diff
Diff
Removed unnecessary diff
(12.73 KB, patch)
2010-01-15 02:12 PST
,
anton muhin
sam
: review-
Details
Formatted Diff
Diff
Added a layout test
(14.53 KB, patch)
2010-01-18 11:34 PST
,
anton muhin
no flags
Details
Formatted Diff
Diff
Patch
(14.84 KB, patch)
2010-03-17 07:51 PDT
,
anton muhin
no flags
Details
Formatted Diff
Diff
Fixing GC layout test to account for new behaviour
(17.47 KB, patch)
2010-03-26 06:10 PDT
,
anton muhin
no flags
Details
Formatted Diff
Diff
Patch
(20.28 KB, patch)
2010-03-26 12:39 PDT
,
anton muhin
no flags
Details
Formatted Diff
Diff
Patch
(20.11 KB, patch)
2010-03-26 13:45 PDT
,
anton muhin
no flags
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
Inlining hasRareData check
(20.54 KB, patch)
2010-03-29 10:49 PDT
,
anton muhin
no flags
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
Addressing Darin's concerns
(26.74 KB, patch)
2010-04-28 12:12 PDT
,
anton muhin
no flags
Details
Formatted Diff
Diff
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 20
2010-01-21 01:52:35 PST
It appears that Firefox (both 3.5.2 and Minefield) actually return the same NodeList repeatedly from getElementsByTagName, in violation of the spec. I was unable to figure out if this is intentional or a bug. Test case:
http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cp%3Etest%3C%2Fp%3E%0A%3Cscript%3E%0Avar%20a%20%3D%20document.getElementsByTagName%28%22p%22%29%3B%0Aa.foo%20%3D%20%22FAIL%22%0Avar%20b%20%3D%20document.getElementsByTagName%28%22p%22%29%3B%0Aw%28%22Same%20object%3A%20%22%20%2B%20%28a%20%3D%3D%3D%20b%29%29%3B%0Aw%28%22foo%20property%3A%20%22%20%2B%20b.foo%29%3B%0A%3C%2Fscript%3E%0A
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.
Top of Page
Format For Printing
XML
Clone This Bug