Bug 95376

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.

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.

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

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?

> 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.

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?

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

(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.

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?

(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.

(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?

(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.

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?

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

(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 :)

(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.

Anyway, thank you folks for the investigation.

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.

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

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

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

Created attachment 165072 [details] Fixes the bug

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.

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

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.

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() ).

Skipped the test in r129564.