Bug 95376 – Dromaeo jquery DOM modification tests run out of memory and crash in current WebKit

• Status: NEW
• Component: DOM

[Boris Zbarsky]

This is almost certainly a regression you guys introduced at some point; this worked the last time I ran WebKit on dromaeo.  Granted, that was a while ago (12 months or more).

In any case, loading the url in the url field and running it will eat all your memory and crash.  I see this in a WebKit nightly on Mac, Chrome on Mac, Chrome on Windows.

Profile of the WebKit nightly on Mac shows mostly layout and some DOM event firing; no idea why memory is being gobbled like no tomorrow.

Note bug 88739 and bug 90598 which are probably about the same thing in your own automation.

[Boris Zbarsky]

Though interestingly, my local copy of this test, pulled from the dromaeo repository many moons ago, works fine in WebKit.

The only non-whitespae difference I see between the two is that my local version is using jquery 1.2.6 (yes, it's old), while the one on dromaeo.com uses jquery 1.6.4.  Pointing my local version at jquery 1.6.4 makes it fail just like the dromaeo.com version.

[Boris Zbarsky]

So it's possible that it's not a regression in WebKit as much as a problem with something jquery 1.6.4 is doing.

I also tried jquery 1.8.0 (current stable tip) and the same OOM happens.

[Boris Zbarsky]

One other thing confusing me:  I think this test is operating on disconnected subtrees.  Why is there layout involved at all?

[Ryosuke Niwa]

Thanks for analysis, Boris!

(In reply to comment #2)
> So it's possible that it's not a regression in WebKit as much as a problem with something jquery 1.6.4 is doing.
> 
> I also tried jquery 1.8.0 (current stable tip) and the same OOM happens.

The last time we analyzed, the problem with the test itself. It doesn't garbage remove the container node it adds, so the test ends up consuming ~4GB of RAM as we run more "runs". I bet the problem is the newer version of jquery is that it's faster, and WebKit is able to run more "runs", consuming more memory.

(In reply to comment #3)
> One other thing confusing me:  I think this test is operating on disconnected subtrees.  Why is there layout involved at all?

It's connected to the document :( But how are you determining whether there is a layout or not?

[Boris Zbarsky]

> It doesn't garbage remove the container node it adds

Which node are you looking at in the test?  The "div"?  I guess that might be in the document, yes....

> I bet the problem is the newer version of jquery is that it's faster, and
> WebKit is able to run more "runs", consuming more memory.

That's plausible; I tried dropping the time each test runs for to 100ms, and then WebKit completes the test, and the scores are higher than with the old jquery.

OK, so it sounds like we should consider changing the test here to test something more reasonable.

> But how are you determining whether there is a layout or not?

I was looking at a profile, remember?  ;)  There was certainly layout-related stuff in there.

[Boris Zbarsky]

So looking at the test some more... on my hardware, the 100ms version runs at about 30000 runs/s in WebKit.  So it would be adding about 120000 elements to the DOM.  That really doesn't seem like it should use up gigabytes of memory.  There are web pages that are bigger than that (e.g. the HTML5 single-page spec).

Ryosuke, would you be willing to double-check the numbers I'm seeing?

[Boris Zbarsky]

Oh, the numbers are off because each jquery API call inserts something like 50 elements, not just one.  One for each div.

[Ryosuke Niwa]

(In reply to comment #7)
> Oh, the numbers are off because each jquery API call inserts something like 50 elements, not just one.  One for each div.

If I remember correctly, the test doubles the number of nodes in each run. So the number of nodes grows exponentially.

[Boris Zbarsky]

I don't see doubling going on here.  The same exact thing is being appended/prepended on each run:

  elemStr = "<strong>some text</strong>";

I just made local changes to the test to clear out the stuff it adds.  With those changes, and numTests=1, WebKit ends up with about 180000 nodes in the document (from the last set of append() calls, which I'm not bothering to clear out) and completes the test.  180000 is a good match for the 3600 runs/s it's scoring on the append() test, given there are 50 divs involved.  So that sounds like the full 5-test run would add/remove about a million nodes to the DOM.  I agree that seems ... excessive.

I wonder whether it would make sense to clear out the kids we added every so many iterations, just as part of the test...  That would certainly confound the measurement of the "append" or whatnot performance, such as it is, though.  Ideas?

[Ryosuke Niwa]

(In reply to comment #9)
> I wonder whether it would make sense to clear out the kids we added every so many iterations, just as part of the test...  That would certainly confound the measurement of the "append" or whatnot performance, such as it is, though.  Ideas?

I differ that question to haraken & morrita.

[Kentaro Hara]

(In reply to comment #10)
> (In reply to comment #9)
> > I wonder whether it would make sense to clear out the kids we added every so many iterations, just as part of the test...  That would certainly confound the measurement of the "append" or whatnot performance, such as it is, though.  Ideas?
> 
> I differ that question to haraken & morrita.

I've been suggesting the same approach:) rniwa wants to upstream the fix to Dromaeo itself because it's a "bug" of Dromaeo.

rniwa: WDYT?

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > I wonder whether it would make sense to clear out the kids we added every so many iterations, just as part of the test...  That would certainly confound the measurement of the "append" or whatnot performance, such as it is, though.  Ideas?
> > 
> > I differ that question to haraken & morrita.
> 
> I've been suggesting the same approach:) rniwa wants to upstream the fix to Dromaeo itself because it's a "bug" of Dromaeo.

Sounds like we should do that then.

Boris, would you be able to make the change upstream? We can re-import the dromaeo tests or cherry pick the fix.

[Boris Zbarsky]

I don't have access to the upstream repo, but I can find out who does.

Do you guys want to write the diff, or should I?

[Boris Zbarsky]

And I agree we want to fix this upstream, for sure.

[Ryosuke Niwa]

(In reply to comment #14)
> And I agree we want to fix this upstream, for sure.

I'm indifferent but as my name indicates, I won't be able to write a patch myself until 9/10. Maybe morrita or haraken would be interested. If not, I'd really appreciate it if you created a patch :)

[Hajime Morrita]

(In reply to comment #15)
> (In reply to comment #14)
> > And I agree we want to fix this upstream, for sure.
> 
> I'm indifferent but as my name indicates, I won't be able to write a patch myself until 9/10. Maybe morrita or haraken would be interested. If not, I'd really appreciate it if you created a patch :)
Dromaeo is hosted on github. So we can just fork
it and send a pullreq for the fix :-)

I'll do it within days if I have time.

[Hajime Morrita]

Anyway, thank you folks for the investigation.

[Boris Zbarsky]

I was thinking the best way to fix without changing the timing is to actually change the harness to allow passing in a cleanup function and cleanup iteration count in addition to the test function, then not counting the time spent in cleanup.

[Ryosuke Niwa]

*** Bug 88739 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

*** Bug 90598 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Uploaded a patch on https://github.com/jeresig/dromaeo/pull/12.

[Ryosuke Niwa]

Created attachment 165072 [details]
Fixes the bug

[WebKit Review Bot]

Attachment 165072 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'PerformanceTests/ChangeLog', u'Performance..." exit_code: 1
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:15:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:32:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:35:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:36:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:37:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:40:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:43:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:44:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:45:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:48:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:50:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:51:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:52:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:53:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:56:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:59:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:60:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:61:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:64:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:67:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:68:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:69:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 22 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

The style error is expected as Dromaeo uses hard tab as opposed to soft tab.

[Hajime Morrita]

Comment on attachment 165072 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=165072&action=review

>> PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:32
>> +	var runs = 0;
> 
> Line contains tab character.  [whitespace/tab] [5]

Setting 0 here doesn't make sense. test() queues the given function up and run it later.
See weberunner.js.

I would add an optional parameter to specify the cleanup function.

[Ryosuke Niwa]

Comment on attachment 165072 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=165072&action=review

>>> PerformanceTests/Dromaeo/resources/dromaeo/web/tests/jslib-modify-jquery.html:32
>>> +	var runs = 0;
>> 
>> Line contains tab character.  [whitespace/tab] [5]
> 
> Setting 0 here doesn't make sense. test() queues the given function up and run it later.
> See weberunner.js.
> 
> I would add an optional parameter to specify the cleanup function.

That's a good point. On the other hand, it's probably fine not to reset runs.
The problem with adding an optional parameter here is that, upstream Dromaeo changes will become much bigger (it has 2-3 different kinds of test() ).

[Ryosuke Niwa]

Skipped the test in r129564.