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

Description anton muhin 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.
Comment 1 anton muhin 2010-01-14 16:02:55 PST
Created attachment 46617 [details]
First take
Comment 2 anton muhin 2010-01-15 02:12:18 PST
Created attachment 46659 [details]
Removed unnecessary diff
Comment 3 anton muhin 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).
Comment 4 anton muhin 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.
Comment 5 Darin Adler 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.
Comment 6 anton muhin 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.
Comment 7 Darin Adler 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?
Comment 8 Sam Weinig 2010-01-16 20:44:02 PST
Comment on attachment 46659 [details]
Removed unnecessary diff

r- since this needs tests due to behavior change.
Comment 9 anton muhin 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).
Comment 10 anton muhin 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?
Comment 11 Sam Weinig 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.
Comment 12 anton muhin 2010-01-18 11:34:50 PST
Created attachment 46836 [details]
Added a layout test
Comment 13 anton muhin 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.
Comment 14 anton muhin 2010-01-19 11:18:48 PST
Guys, so what do you think about this change?
Comment 15 anton muhin 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.
Comment 16 anton muhin 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)
Comment 17 Maciej Stachowiak 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.
Comment 18 Maciej Stachowiak 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.
Comment 19 Maciej Stachowiak 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.
Comment 21 Maciej Stachowiak 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.
Comment 22 anton muhin 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.
Comment 23 Maciej Stachowiak 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.
Comment 24 anton muhin 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.
Comment 25 anton muhin 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?
Comment 26 anton muhin 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.
Comment 27 Maciej Stachowiak 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.
Comment 28 anton muhin 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?
Comment 29 anton muhin 2010-03-17 07:51:12 PDT
Created attachment 50906 [details]
Patch
Comment 30 anton muhin 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?
Comment 31 Maciej Stachowiak 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.
Comment 32 Oliver Hunt 2010-03-25 23:03:56 PDT
Comment on attachment 50906 [details]
Patch

r=me
Comment 33 anton muhin 2010-03-26 03:35:25 PDT
Comment on attachment 50906 [details]
Patch

Many thanks to everyone!
Comment 34 WebKit Commit Bot 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
Comment 35 anton muhin 2010-03-26 06:10:11 PDT
Created attachment 51734 [details]
Fixing GC layout test to account for new behaviour
Comment 36 Darin Adler 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.
Comment 37 anton muhin 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?
Comment 38 Darin Adler 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*.
Comment 39 anton muhin 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.
Comment 40 Darin Adler 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.
Comment 41 anton muhin 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.
Comment 42 anton muhin 2010-03-26 12:39:56 PDT
Created attachment 51772 [details]
Patch
Comment 43 anton muhin 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.
Comment 44 Darin Adler 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.
Comment 45 Darin Adler 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.)
Comment 46 anton muhin 2010-03-26 13:45:56 PDT
Created attachment 51776 [details]
Patch
Comment 47 anton muhin 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.
Comment 48 Darin Adler 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
Comment 49 anton muhin 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.
Comment 50 Darin Adler 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.
Comment 51 WebKit Commit Bot 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
Comment 52 Eric Seidel (no email) 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.
Comment 53 Eric Seidel (no email) 2010-03-27 17:30:56 PDT
The system deleted the crash report sadly, due to too many other crashes since then.
Comment 54 WebKit Commit Bot 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
Comment 55 WebKit Commit Bot 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
Comment 56 Eric Seidel (no email) 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. :(
Comment 57 Eric Seidel (no email) 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).
Comment 58 Eric Seidel (no email) 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.
Comment 59 WebKit Commit Bot 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
Comment 60 Alexey Proskuryakov 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.
Comment 61 anton muhin 2010-03-29 08:07:43 PDT
Created attachment 51911 [details]
Patch
Comment 62 WebKit Review Bot 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.
Comment 63 anton muhin 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?)
Comment 64 anton muhin 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())?
Comment 65 anton muhin 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.
Comment 66 Alexey Proskuryakov 2010-03-29 08:45:52 PDT
I believe it crashes on Mac. QT is QuickTime, Qt is Nokia.
Comment 67 Darin Adler 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.
Comment 68 Darin Adler 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.
Comment 69 Adam Barth 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>?
Comment 70 Darin Adler 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.
Comment 71 Adam Barth 2010-03-29 09:16:48 PDT
> But there should be.

https://bugs.webkit.org/show_bug.cgi?id=36760
Comment 72 anton muhin 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?
Comment 73 anton muhin 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.
Comment 74 anton muhin 2010-03-29 10:49:28 PDT
Created attachment 51930 [details]
Inlining hasRareData check
Comment 75 anton muhin 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.
Comment 76 WebKit Review Bot 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.
Comment 77 Eric Seidel (no email) 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
Comment 78 Eric Seidel (no email) 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.
Comment 79 Eric Seidel (no email) 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.
Comment 80 Eric Seidel (no email) 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.
+
+
Comment 81 Eric Seidel (no email) 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.
Comment 82 Eric Seidel (no email) 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.
Comment 83 Eric Seidel (no email) 2010-04-01 17:25:59 PDT
I failed to repro the crash on my other mac pro.
Comment 84 Eric Seidel (no email) 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.
Comment 85 Alexey Proskuryakov 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.
Comment 86 anton muhin 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
Comment 87 Eric Seidel (no email) 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.
Comment 88 Alexey Proskuryakov 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.
Comment 89 Eric Seidel (no email) 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
Comment 90 Alexey Proskuryakov 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.
Comment 91 Eric Seidel (no email) 2010-04-02 13:15:18 PDT
YAY!  The commit-bot is not crazy.  Thank you Alexey.  Ball is in your court Anton. :)
Comment 92 anton muhin 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!
Comment 93 Alexey Proskuryakov 2010-04-05 08:42:03 PDT
I ran the tests on an 8 core Mac Pro.
Comment 94 anton muhin 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)?
Comment 95 Alexey Proskuryakov 2010-04-12 18:18:26 PDT
This is unlikely to be related. The crashes I was getting were on fast/dom tests.
Comment 96 anton muhin 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?
Comment 97 Alexey Proskuryakov 2010-04-13 09:21:14 PDT
Since fast/dom goes alphabetically before media, DOM tests should run earlier than media ones.
Comment 98 anton muhin 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.
Comment 99 anton muhin 2010-04-27 12:23:51 PDT
Created attachment 54443 [details]
Patch
Comment 100 anton muhin 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.
Comment 101 WebKit Review Bot 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.
Comment 102 Darin Adler 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.
Comment 103 anton muhin 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.
Comment 104 Darin Adler 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.
Comment 105 anton muhin 2010-04-28 12:12:39 PDT
Created attachment 54604 [details]
Addressing Darin's concerns
Comment 106 WebKit Review Bot 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.
Comment 107 anton muhin 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)
Comment 108 anton muhin 2010-04-29 08:15:57 PDT
Comment on attachment 54604 [details]
Addressing Darin's concerns

Thanks a lot, Daring, cq+'ing it.
Comment 109 WebKit Commit Bot 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>
Comment 110 WebKit Commit Bot 2010-04-29 11:03:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 111 Alexey Proskuryakov 2010-06-11 15:21:29 PDT
*** Bug 31308 has been marked as a duplicate of this bug. ***
Comment 112 Alexey Proskuryakov 2010-06-11 15:22:17 PDT
This fix was incomplete, see bug 9508 for a case where we still diverge from Firefox.
Comment 113 anton muhin 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.
Comment 114 Alexey Proskuryakov 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.