95376 2012-08-29 14:02:13 -0700 Dromaeo jquery DOM modification tests run out of memory and crash in current WebKit 2012-09-25 15:52:17 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Unspecified Unspecified NEW http://dromaeo.com/?modify-jquery P2 Normal --- 93185 1 bzbarsky rniwa ap haraken morrita rniwa webkit.review.bot yosin oldest_to_newest 707837 0 bzbarsky 2012-08-29 14:02:13 -0700 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. 707847 1 bzbarsky 2012-08-29 14:11:47 -0700 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. 707864 2 bzbarsky 2012-08-29 14:22:38 -0700 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. 707867 3 bzbarsky 2012-08-29 14:24:05 -0700 One other thing confusing me: I think this test is operating on disconnected subtrees. Why is there layout involved at all? 707992 4 rniwa 2012-08-29 16:06:14 -0700 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? 708131 5 bzbarsky 2012-08-29 18:06:00 -0700 > 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. 708140 6 bzbarsky 2012-08-29 18:09:31 -0700 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? 708152 7 bzbarsky 2012-08-29 18:14:18 -0700 Oh, the numbers are off because each jquery API call inserts something like 50 elements, not just one. One for each div. 708163 8 rniwa 2012-08-29 18:26:36 -0700 (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. 708182 9 bzbarsky 2012-08-29 18:40:20 -0700 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? 708185 10 rniwa 2012-08-29 18:44:48 -0700 (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. 708187 11 haraken 2012-08-29 18:48:25 -0700 (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? 708190 12 rniwa 2012-08-29 18:53:12 -0700 (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. 708208 13 bzbarsky 2012-08-29 19:05:18 -0700 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? 708209 14 bzbarsky 2012-08-29 19:05:44 -0700 And I agree we want to fix this upstream, for sure. 708212 15 rniwa 2012-08-29 19:08:10 -0700 (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 :) 708300 16 morrita 2012-08-29 22:13:59 -0700 (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. 708302 17 morrita 2012-08-29 22:14:54 -0700 Anyway, thank you folks for the investigation. 708538 18 bzbarsky 2012-08-30 05:55:43 -0700 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. 725078 19 rniwa 2012-09-20 19:06:41 -0700 *** Bug 88739 has been marked as a duplicate of this bug. *** 725080 20 rniwa 2012-09-20 19:06:49 -0700 *** Bug 90598 has been marked as a duplicate of this bug. *** 725236 21 rniwa 2012-09-21 00:47:40 -0700 Uploaded a patch on https://github.com/jeresig/dromaeo/pull/12. 725239 22 165072 rniwa 2012-09-21 00:50:22 -0700 Created attachment 165072 Fixes the bug 725240 23 webkit.review.bot 2012-09-21 00:53:56 -0700 Attachment 165072 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. 725722 24 rniwa 2012-09-21 12:47:00 -0700 The style error is expected as Dromaeo uses hard tab as opposed to soft tab. 727188 25 165072 morrita 2012-09-24 18:54:04 -0700 Comment on attachment 165072 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. 727211 26 165072 rniwa 2012-09-24 19:50:53 -0700 Comment on attachment 165072 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() ). 728039 27 rniwa 2012-09-25 15:52:17 -0700 Skipped the test in r129564. 165072 2012-09-21 00:50:22 -0700 2012-09-24 19:50:53 -0700 Fixes the bug bug-95376-20120921004947.patch text/plain 2697 rniwa SW5kZXg6IFBlcmZvcm1hbmNlVGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFBlcmZvcm1h bmNlVGVzdHMvQ2hhbmdlTG9nCShyZXZpc2lvbiAxMjkyMDApCisrKyBQZXJmb3JtYW5jZVRlc3Rz L0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDEyLTA5LTIxICBS eW9zdWtlIE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIERyb21hZW8ganF1ZXJ5 IERPTSBtb2RpZmljYXRpb24gdGVzdHMgcnVuIG91dCBvZiBtZW1vcnkgYW5kIGNyYXNoIGluIGN1 cnJlbnQgV2ViS2l0CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD05NTM3NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFJlbW92ZSBub2RlcyBhZGRlZCBieSBlYWNoIHRlc3QgY2FzZSBldmVyeSAxMDAwIHJ1bnMg c28gdGhhdCB3ZSBkb24ndCBlbmQgdXAgd2l0aAorICAgICAgICAxZSs3IG9yIDFlKzggbm9kZXMg YW5kIHVzZSB1cCB0aGUgZW50aXJlIDRHQiBvZiBSQU0gaW4gQ2hyb21pdW0uCisKKyAgICAgICAg VG8gZG8gdGhpcywgY2hhbmdlZCB0aGUgbmFtZSBvZiBlbGVtZW50IHdlIGluc2VydCBmcm9tIHN0 cm9uZyB0byBzdHJpa2Ugc28gdGhhdAorICAgICAgICB3ZSBjYW4gcXVpY2tseSBlbnVtZXJhdGUg YWxsIHRoZSBlbGVtZW50cyB0byByZW1vdmUgKHN0cmlrZSBkb2Vzbid0IGFwcGVhciBlbHNld2hl cmUgaW4gdGhlIGRvY3VtZW50KS4KKworICAgICAgICBBbHNvLCBjYWxsIGFmdGVyKCkgaW4gdGhl IHRlc3QgY2FzZSBsYWJlbGVkICJqUXVlcnkgLSBhZnRlcigpIi4KKworICAgICAgICBUaGlzIGNo YW5nZSBpcyBiZWluZyBjb250cmlidXRlZCBiYWNrIGF0IGh0dHBzOi8vZ2l0aHViLmNvbS9qZXJl c2lnL2Ryb21hZW8vcHVsbC8xMi4KKworICAgICAgICAqIERyb21hZW8vcmVzb3VyY2VzL2Ryb21h ZW8vd2ViL3Rlc3RzL2pzbGliLW1vZGlmeS1qcXVlcnkuaHRtbDoKKwogMjAxMi0wOS0yMCAgUnlv c3VrZSBOaXdhICA8cm5pd2FAd2Via2l0Lm9yZz4KIAogICAgICAgICBSZXN1bHRzIHBhZ2Ugc2hv dWxkIHNob3cgaW5kaXZpc3VhbCB2YWx1ZQpJbmRleDogUGVyZm9ybWFuY2VUZXN0cy9Ecm9tYWVv L3Jlc291cmNlcy9kcm9tYWVvL3dlYi90ZXN0cy9qc2xpYi1tb2RpZnktanF1ZXJ5Lmh0bWwKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gUGVyZm9ybWFuY2VUZXN0cy9Ecm9tYWVvL3Jlc291cmNlcy9kcm9tYWVvL3dl Yi90ZXN0cy9qc2xpYi1tb2RpZnktanF1ZXJ5Lmh0bWwJKHJldmlzaW9uIDEyOTE2NCkKKysrIFBl cmZvcm1hbmNlVGVzdHMvRHJvbWFlby9yZXNvdXJjZXMvZHJvbWFlby93ZWIvdGVzdHMvanNsaWIt bW9kaWZ5LWpxdWVyeS5odG1sCSh3b3JraW5nIGNvcHkpCkBAIC0xMiw3ICsxMiw3IEBAIHZhciBy ZXQsIHRtcCwgZGl2LCBhLCBkZDsKIHZhciBodG1sID0gZG9jdW1lbnQuYm9keS5pbm5lckhUTUws CiAJZWxlbSA9IGRvY3VtZW50LmNyZWF0ZUVsZW1lbnQoImRpdiIpLAogCWVsZW0yID0gZG9jdW1l bnQuY3JlYXRlRWxlbWVudCgic3Ryb25nIiksCi0JZWxlbVN0ciA9ICI8c3Ryb25nPnNvbWUgdGV4 dDwvc3Ryb25nPiI7CisJZWxlbVN0ciA9ICI8c3RyaWtlPnNvbWUgdGV4dDwvc3RyaWtlPiI7CiAK IAlwcmVwKGZ1bmN0aW9uKCl7CiAJCWEgPSBqUXVlcnkoImEiKTsKQEAgLTI5LDI0ICsyOSw0NCBA QCB2YXIgaHRtbCA9IGRvY3VtZW50LmJvZHkuaW5uZXJIVE1MLAogCX0pOwogCSovCiAKKwl2YXIg cnVucyA9IDA7CiAJdGVzdCgialF1ZXJ5IC0gaHRtbCgpIiwgZnVuY3Rpb24oKXsKIAkJZGQuaHRt bCggZWxlbVN0ciApOworCQlydW5zKys7CisJCWlmIChydW5zICUgMTAwMCA9PSAwKQorCQkJalF1 ZXJ5KCdzdHJpa2UnKS5yZW1vdmUoKTsKIAl9KTsKIAorCXJ1bnMgPSAwOwogCXRlc3QoImpRdWVy eSAtIGJlZm9yZSgpIiwgZnVuY3Rpb24oKXsKIAkJZGl2LmJlZm9yZSggZWxlbVN0ciApOworCQly dW5zKys7CisJCWlmIChydW5zICUgMTAwMCA9PSAwKQorCQkJalF1ZXJ5KCdzdHJpa2UnKS5yZW1v dmUoKTsKIAl9KTsKIAorCXJ1bnMgPSAwOwogCXRlc3QoImpRdWVyeSAtIGFmdGVyKCkiLCBmdW5j dGlvbigpewotCQlkaXYuYmVmb3JlKCBlbGVtU3RyICk7CisJCWRpdi5hZnRlciggZWxlbVN0ciAp OworCQlydW5zKys7CisJCWlmIChydW5zICUgMTAwMCA9PSAwKQorCQkJalF1ZXJ5KCdzdHJpa2Un KS5yZW1vdmUoKTsKIAl9KTsKIAorCXJ1bnMgPSAwOwogCXRlc3QoImpRdWVyeSAtIHByZXBlbmQo KSIsIGZ1bmN0aW9uKCl7CiAJCWRpdi5wcmVwZW5kKCBlbGVtU3RyICk7CisJCXJ1bnMrKzsKKwkJ aWYgKHJ1bnMgJSAxMDAwID09IDApCisJCQlqUXVlcnkoJ3N0cmlrZScpLnJlbW92ZSgpOwogCX0p OwogCisJcnVucyA9IDA7CiAJdGVzdCgialF1ZXJ5IC0gYXBwZW5kKCkiLCBmdW5jdGlvbigpewog CQlkaXYuYXBwZW5kKCBlbGVtU3RyICk7CisJCXJ1bnMrKzsKKwkJaWYgKHJ1bnMgJSAxMDAwID09 IDApCisJCQlqUXVlcnkoJ3N0cmlrZScpLnJlbW92ZSgpOwogCX0pOwogCiAJLyogLy8gTmVlZCBh IGdvb2Qgd2F5IHRvIHRlc3QK