Bug 79555 – JSString::resolveRope() should report extra memory cost to heap

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yong Li]

JSString::resolveRope() should report extra memory cost to heap, otherwise, it can silently boost memory usage but never trigger a GC.

Patch is forthcoming

[Yong Li]

Created attachment 128845 [details]
the patch

[Yong Li]

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.

[Geoffrey Garen]

This looks correct to me, but I'd like to see performance results before saying r+.

[Yong Li]

Created attachment 129102 [details]
a test case (just load it in Safari)

A test case that shows how bad this issue can be.

[Yong Li]

Comment on attachment 129102 [details]
a test case (just load it in Safari)

sorry this isn't right

[Geoffrey Garen]

What I meant was performance results from running Tools/Scripts/bencher.

[Michael Saboff]

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.

[Yong Li]

I was just going to do it this morning, but already see the performance results :). Thanks a lot!

[WebKit Review Bot]

Comment on attachment 128845 [details]
the patch

Clearing flags on attachment: 128845

Committed r109105: <http://trac.webkit.org/changeset/109105>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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'

[Yong Li]

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

[Yong Li]

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

[Yong Li]

Never mind.

    if (message_event.data !== kPrefix + num.toString()) {

Probably "kPrefix + num.toString()" got GC'ed...

[Geoffrey Garen]

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

[Csaba Osztrogonác]

I skipped it on Qt to paint the bot green - http://trac.webkit.org/changeset/109202 . Please unskip it once the bug is fixed.

[Yong Li]

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

[Yong Li]

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

[Yong Li]

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

[Philippe Normand]

The very same failure happens on GTK bots as well.

[Csaba Osztrogonác]

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

[Yong Li]

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

[Philippe Normand]

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

[Yong Li]

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

[Yong Li]

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);"