JSString::resolveRope() should report extra memory cost to heap, otherwise, it can silently boost memory usage but never trigger a GC. Patch is forthcoming
Created attachment 128845 [details] the patch
BTW, I know the fibers will be cleared when rope is resolved. However the fibers are also JSString. Even they are the only owners of the string buffers, those string buffers won't be released until next GC. So it is even more necessary to notify heap for the new memory cost.
This looks correct to me, but I'd like to see performance results before saying r+.
Created attachment 129102 [details] a test case (just load it in Safari) A test case that shows how bad this issue can be.
Comment on attachment 129102 [details] a test case (just load it in Safari) sorry this isn't right
What I meant was performance results from running Tools/Scripts/bencher.
I ran bencher with the change: VMs tested: "Baseline" at /Volumes/Data/src/webkit.baseline/WebKitBuild/Release/jsc (r109029) "With79555" at /Volumes/Data/src/webkit/WebKitBuild/Release/jsc (r109029) Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. Baseline With79555 SunSpider: 3d-cube 5.8065+-0.0636 5.7976+-0.0348 3d-morph 9.7651+-0.0655 9.7475+-0.0584 3d-raytrace 7.8058+-0.0845 7.7804+-0.1359 access-binary-trees 1.6814+-0.0083 ? 1.6891+-0.0117 ? access-fannkuch 7.4542+-0.0378 ? 7.4615+-0.0387 ? access-nbody 3.8650+-0.0121 ? 3.8658+-0.0083 ? access-nsieve 3.4944+-0.0348 ? 3.4945+-0.0369 ? bitops-3bit-bits-in-byte 1.2890+-0.0116 1.2827+-0.0024 bitops-bits-in-byte 5.2574+-0.0148 ? 5.2639+-0.0279 ? bitops-bitwise-and 3.3042+-0.0109 3.3022+-0.0062 bitops-nsieve-bits 3.3366+-0.0121 3.3241+-0.0027 controlflow-recursive 2.3364+-0.0095 ? 2.3375+-0.0161 ? crypto-aes 7.5264+-0.0598 ? 7.5782+-0.0733 ? crypto-md5 2.8876+-0.0368 2.8643+-0.0226 crypto-sha1 2.4409+-0.0311 2.4334+-0.0269 date-format-tofte 11.0680+-0.1543 10.8679+-0.0523 might be 1.0184x faster date-format-xparb 10.3282+-0.1265 ! 10.8125+-0.2257 ! definitely 1.0469x slower math-cordic 7.5014+-0.0586 ? 7.5187+-0.0581 ? math-partial-sums 10.5861+-0.0533 ? 10.5951+-0.0332 ? math-spectral-norm 2.6737+-0.0134 2.6717+-0.0041 regexp-dna 8.9740+-0.0569 ? 9.0282+-0.0640 ? string-base64 4.3738+-0.0134 ? 4.3838+-0.0200 ? string-fasta 7.2748+-0.0436 ? 7.2940+-0.0623 ? string-tagcloud 12.8811+-0.0680 ! 13.0507+-0.0959 ! definitely 1.0132x slower string-unpack-code 22.0923+-0.1550 21.9631+-0.1851 string-validate-input 6.5372+-0.0665 6.4888+-0.0790 <arithmetic> * 6.6362+-0.0224 ? 6.6499+-0.0168 ? might be 1.0021x slower <geometric> 5.3565+-0.0173 ? 5.3622+-0.0106 ? might be 1.0011x slower <harmonic> 4.2590+-0.0164 4.2576+-0.0088 might be 1.0003x faster Baseline With79555 V8: crypto 75.2921+-0.2420 75.1662+-0.2582 deltablue 159.3357+-1.0798 158.7000+-0.7260 earley-boyer 99.7900+-0.4689 ? 100.4316+-0.6263 ? raytrace 52.0263+-0.3561 ? 52.2842+-0.5627 ? regexp 101.9057+-0.4743 ? 102.1308+-0.4548 ? richards 145.3947+-1.1873 144.3771+-1.0141 splay 59.5547+-0.2939 59.2430+-0.2968 <arithmetic> 99.0427+-0.2055 98.9047+-0.1527 might be 1.0014x faster <geometric> * 91.8010+-0.1860 91.7429+-0.1588 might be 1.0006x faster <harmonic> 85.0683+-0.1982 85.0617+-0.1961 might be 1.0001x faster Baseline With79555 Kraken: ai-astar 809.4562+-13.3802 ? 821.8329+-11.7089 ? might be 1.0153x slower audio-beat-detection 192.9189+-2.0074 ^ 190.4819+-0.3250 ^ definitely 1.0128x faster audio-dft 291.9583+-4.6616 289.0048+-2.0882 might be 1.0102x faster audio-fft 116.8073+-0.1773 ? 116.8932+-0.6872 ? audio-oscillator 301.8988+-1.1253 301.6176+-1.3546 imaging-darkroom 293.9836+-6.5322 ? 296.2939+-6.6740 ? imaging-desaturate 237.5340+-0.2004 ^ 237.1890+-0.1405 ^ definitely 1.0015x faster imaging-gaussian-blur 455.1080+-0.1737 ? 455.4429+-0.3213 ? json-parse-financial 64.0643+-0.1497 ^ 63.2265+-0.2037 ^ definitely 1.0133x faster json-stringify-tinderbox 76.9872+-0.4034 ? 77.1329+-0.3894 ? stanford-crypto-aes 104.1311+-0.9987 102.9684+-0.6176 might be 1.0113x faster stanford-crypto-ccm 100.6942+-0.6070 100.4644+-0.6231 stanford-crypto-pbkdf2 200.0193+-0.7368 ? 200.7508+-0.6206 ? stanford-crypto-sha256-iterative 90.7559+-0.4278 90.5753+-0.2473 <arithmetic> * 238.3083+-1.0552 ? 238.8482+-0.9982 ? might be 1.0023x slower <geometric> 183.1813+-0.5011 182.8738+-0.3854 might be 1.0017x faster <harmonic> 146.3076+-0.3062 ^ 145.7417+-0.2077 ^ definitely 1.0039x faster Baseline With79555 All benchmarks: <arithmetic> 89.4076+-0.3439 ? 89.5554+-0.2991 ? might be 1.0017x slower <geometric> 23.4206+-0.0635 23.4204+-0.0390 might be 1.0000x faster <harmonic> 7.4808+-0.0283 7.4781+-0.0153 might be 1.0004x faster Baseline With79555 Geomean of preferred means: <scaled-result> 52.5575+-0.1566 ? 52.6220+-0.1053 ? might be 1.0012x slower It appears that there is a slight slowdown in sun spider.string-tagcloud (~1%). I verified there was a slowdown for tagclound os ~1% using run-sunspider harness. Overall this has a .1% impact on sun spider or all three tests, within the noise band.
I was just going to do it this morning, but already see the performance results :). Thanks a lot!
Comment on attachment 128845 [details] the patch Clearing flags on attachment: 128845 Committed r109105: <http://trac.webkit.org/changeset/109105>
All reviewed patches have been landed. Closing bug.
Reopen, because it broke fast/events/dispatch-message-string-data.html on Qt and on GTK: --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/dispatch-message-string-data-expected.txt +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/dispatch-message-string-data-actual.txt @@ -1,4 +1,5 @@ This is a test for https://bugs.webkit.org/show_bug.cgi?id=71229 (V8MessageEvent::dataAccessorGetter does not return a reference to its caller). If it succeeds, DONE will appear below. If it fails, you should see messages containing unexpected strings that were received and/or a renderer crash. DONE +Expected '818', Got: 'HelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHelloHello818'
(In reply to comment #11) > Reopen, because it broke fast/events/dispatch-message-string-data.html on Qt and on GTK: > > --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/dispatch-message-string-data-expected.txt > +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/dispatch-message-string-data-actual.txt > @@ -1,4 +1,5 @@ > This is a test for https://bugs.webkit.org/show_bug.cgi?id=71229 (V8MessageEvent::dataAccessorGetter does not return a reference to its caller). If it succeeds, DONE will appear below. If it fails, you should see messages containing unexpected strings that were received and/or a renderer crash. > > DONE > +Expected '818', Got: This must be an existing issue but triggered by this patch. heap->reportExtraMemoryCost(n) shouldn't be problem no matter how much n is.
(In reply to comment #11) > Reopen, because it broke fast/events/dispatch-message-string-data.html on Qt and on GTK: > > --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/dispatch-message-string-data-expected.txt > +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/dispatch-message-string-data-actual.txt > @@ -1,4 +1,5 @@ > This is a test for https://bugs.webkit.org/show_bug.cgi?id=71229 (V8MessageEvent::dataAccessorGetter does not return a reference to its caller). If it succeeds, DONE will appear below. If it fails, you should see messages containing unexpected strings that were received and/or a renderer crash. > > DONE > +Expected '818', Got: 'Hello... It is 1024 "Hello"s + 808. The JS: var kPrefix = "Hello"; for (var i = 0; i < 10; ++i) kPrefix += kPrefix; 2^10 = 1024 if (message_event.data !== kPrefix + num.toString()) { log("Expected '" + num + "', Got: '" + message_event.data + "'"); } So the message_event.data seems right. what's "!=="? is it a supported operator in JS?
Never mind. if (message_event.data !== kPrefix + num.toString()) { Probably "kPrefix + num.toString()" got GC'ed...
> This must be an existing issue but triggered by this patch. Not necessarily. For example, you may have introduced a garbage collection at a time when your string is in an invalid state. If you don't have any immediate debugging leads, I think the best next step is to roll the patch out and rework it.
I skipped it on Qt to paint the bot green - http://trac.webkit.org/changeset/109202 . Please unskip it once the bug is fixed.
(In reply to comment #15) > > This must be an existing issue but triggered by this patch. > > Not necessarily. For example, you may have introduced a garbage collection at a time when your string is in an invalid state. > > If you don't have any immediate debugging leads, I think the best next step is to roll the patch out and rework it. If that is true, it is a serious problem. I'll try to fix it today. I'm not against rolling it out if it is necessary.
(In reply to comment #16) > I skipped it on Qt to paint the bot green - http://trac.webkit.org/changeset/109202 . Please unskip it once the bug is fixed. Thanks, Csaba. However I cannot reproduce the problem even by reporting 64MB memory cost which for sure triggers a GC. This might be port-specific issue. I'll try to make a qt build, but probably it will take me some time. Do you know any qt/gtk webkit developer available to help investigating this issue?
(In reply to comment #11) > Reopen, because it broke fast/events/dispatch-message-string-data.html on Qt and on GTK: > > --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/dispatch-message-string-data-expected.txt > +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/dispatch-message-string-data-actual.txt > @@ -1,4 +1,5 @@ > This is a test for https://bugs.webkit.org/show_bug.cgi?id=71229 (V8MessageEvent::dataAccessorGetter does not return a reference to its caller). If it succeeds, DONE will appear below. If it fails, you should see messages containing unexpected strings that were received and/or a renderer crash. > > DONE > +Expected '818', Got: .. Csaba, finally we got Qt 5.0 webkit release build on ubuntu (32-bit), but it passed the test, even after I made it reportExtraCost(64*1024*1024). Given that the issue cannot be produced on either ubuntu 32-bit or QNX, I tend to suggest it is a platform/port specific GC issue, probably something related to stack walking code, and probably related to 64-bit linux.
The very same failure happens on GTK bots as well.
(In reply to comment #19) > (In reply to comment #11) > > Reopen, because it broke fast/events/dispatch-message-string-data.html on Qt and on GTK: > > > > --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/dispatch-message-string-data-expected.txt > > +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/events/dispatch-message-string-data-actual.txt > > @@ -1,4 +1,5 @@ > > This is a test for https://bugs.webkit.org/show_bug.cgi?id=71229 (V8MessageEvent::dataAccessorGetter does not return a reference to its caller). If it succeeds, DONE will appear below. If it fails, you should see messages containing unexpected strings that were received and/or a renderer crash. > > > > DONE > > +Expected '818', Got: > .. > > Csaba, finally we got Qt 5.0 webkit release build on ubuntu (32-bit), but it passed the test, even after I made it reportExtraCost(64*1024*1024). Given that the issue cannot be produced on either ubuntu 32-bit or QNX, I tend to suggest it is a platform/port specific GC issue, probably something related to stack walking code, and probably related to 64-bit linux. Sorry, I didn't realize that this test fails only on 64 bit (Qt, GTK too), but pass on 32 bit. And tested with Qt 4.8.0. But I think it should fail with Qt 5 on 64 bit too. I'll check it later.
(In reply to comment #20) > The very same failure happens on GTK bots as well. Is it also 64-bit linux? There isn't difference between QT and GTK in JS engine as long as the OS and CPU are same. I tried the instructions of building GTK port. But it fails "due to non buildable" gdk-pixbuf and other libs. Any idea?
(In reply to comment #22) > (In reply to comment #20) > > The very same failure happens on GTK bots as well. > > Is it also 64-bit linux? There isn't difference between QT and GTK in JS engine as long as the OS and CPU are same. I tried the instructions of building GTK port. But it fails "due to non buildable" gdk-pixbuf and other libs. Any idea? Yes, I think it was only happening on the 2 64-bit GTK bots. For your build issues maybe we can discuss on IRC? My nickname is philn-tp.
Actually reporting 64MB cost doesn't always trigger GC. We modified reportExtraMemoryCostSlowCase() to always do a GC instead. And we cannot see any problem on Mac 64-bit (Safari), Linux 32-bit (Qt) and QNX. (Many thanks to Jacky Jiang's help for making qtwebkit build on linux and safari build on Mac.)
Can someone who (In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #20) > > > The very same failure happens on GTK bots as well. > > > > Is it also 64-bit linux? There isn't difference between QT and GTK in JS engine as long as the OS and CPU are same. I tried the instructions of building GTK port. But it fails "due to non buildable" gdk-pixbuf and other libs. Any idea? > > Yes, I think it was only happening on the 2 64-bit GTK bots. > For your build issues maybe we can discuss on IRC? My nickname is philn-tp. Is this problem still happening? There seems to be a pthread mutex issue on some 64bit linux, which I think can lead to any kind of weird crash. https://bugs.webkit.org/show_bug.cgi?id=88419 Could you try this change (suggested by Joe)? > As a quick hack, try changing "PlatformMutex m_mutex;" in > ThreadingPrimitives.h to "WTF_ALIGNED(PlatformMutex, m_mutex, 64);"