Bug 84907 – Memory wasted in JSString for non-rope strings

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

JSString effectively has four pointers to data, a UString, which contains a pointer to a StringImpl, and 3 pointers to other JSStrings in the case the string is a rope made up of other strings.

For strings created from a UString or as a substring of another JSString, the three "fiber" pointers are not used.

We can save the space of these three pointers by splitting JSString into a class that has a UString and another class that is a rope.

[Michael Saboff]

Created attachment 138902 [details]
Proposed Patch

This appears overall neutral on performance tests.  The only test that looks like it might slow down in SunSpider string-base64.  Other SunSpider tests seem to offset that loss so that SunSpider nets out with no change.

Benchmark report for SunSpider, V8, and Kraken on msaboff-pro (MacPro5,1).

VMs tested:
"Baseline" at /Volumes/Data/src/webkit.baseline/WebKitBuild/Release/jsc (r115220)
"StringSplit" at /Volumes/Data/src/webkit.work/WebKitBuild/Release/jsc (r115220)

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              StringSplit                                   
SunSpider:
   3d-cube                                7.1923+-0.0878          7.1591+-0.1012       
   3d-morph                               7.3342+-0.1166    ?     7.3962+-0.0787       ?
   3d-raytrace                            9.6550+-0.1114          9.5425+-0.0897         might be 1.0118x faster
   access-binary-trees                    1.8062+-0.0146    ?     1.8078+-0.0155       ?
   access-fannkuch                        7.4821+-0.0565    ?     7.5589+-0.0891       ? might be 1.0103x slower
   access-nbody                           3.8797+-0.0511    ?     3.8995+-0.0594       ?
   access-nsieve                          3.6100+-0.0518    ?     3.6761+-0.0453       ? might be 1.0183x slower
   bitops-3bit-bits-in-byte               1.3877+-0.0126          1.3839+-0.0155       
   bitops-bits-in-byte                    5.4218+-0.0903          5.3617+-0.0706         might be 1.0112x faster
   bitops-bitwise-and                     3.4084+-0.0385    ?     3.4516+-0.0456       ? might be 1.0127x slower
   bitops-nsieve-bits                     3.3156+-0.0421    !     3.3741+-0.0106       ! definitely 1.0176x slower
   controlflow-recursive                  2.4107+-0.0339    ?     2.4555+-0.0269       ? might be 1.0185x slower
   crypto-aes                             8.1091+-0.0905          8.0742+-0.1588       
   crypto-md5                             3.3620+-0.0474          3.3256+-0.0533         might be 1.0110x faster
   crypto-sha1                            2.6600+-0.0604    ?     2.6639+-0.0488       ?
   date-format-tofte                     11.1965+-0.2238    ?    11.2499+-0.1706       ?
   date-format-xparb                     11.0137+-0.1682         10.7141+-0.1453         might be 1.0280x faster
   math-cordic                            4.1943+-0.0542          4.1473+-0.0333         might be 1.0113x faster
   math-partial-sums                      9.7927+-0.1006          9.7874+-0.0972       
   math-spectral-norm                     2.8539+-0.0344          2.8480+-0.0384       
   regexp-dna                             9.7621+-0.0957    ?     9.7827+-0.1204       ?
   string-base64                          4.7095+-0.0715    !     4.8647+-0.0832       ! definitely 1.0330x slower
   string-fasta                           7.3884+-0.0862          7.3108+-0.1465         might be 1.0106x faster
   string-tagcloud                       12.9977+-0.1691         12.9032+-0.1906       
   string-unpack-code                    21.6388+-0.3067         21.5812+-0.2760       
   string-validate-input                  6.6081+-0.1529          6.5152+-0.1037         might be 1.0143x faster

   <arithmetic> *                         6.6612+-0.0349          6.6475+-0.0364         might be 1.0021x faster
   <geometric>                            5.4346+-0.0310    ?     5.4368+-0.0296       ? might be 1.0004x slower
   <harmonic>                             4.3992+-0.0283    ?     4.4095+-0.0260       ? might be 1.0023x slower

                                             Baseline              StringSplit                                   
V8:
   crypto                                75.6874+-0.6223         75.3308+-0.5333       
   deltablue                            154.9205+-1.1508    ?   155.4980+-1.4269       ?
   earley-boyer                          95.1838+-1.3352         94.8811+-1.4211       
   raytrace                              53.1343+-0.2090    ?    53.3965+-0.4181       ?
   regexp                                95.1276+-0.4268         94.7119+-0.4922       
   richards                             141.0346+-1.0650        139.7111+-0.5126       
   splay                                 90.4939+-11.5016   ?    90.6910+-10.9277      ?

   <arithmetic>                         100.7975+-1.8471        100.6029+-1.8123         might be 1.0019x faster
   <geometric> *                         95.2155+-1.8432         95.0883+-1.8053         might be 1.0013x faster
   <harmonic>                            89.7821+-1.6562         89.7419+-1.6340         might be 1.0004x faster

                                             Baseline              StringSplit                                   
Kraken:
   ai-astar                             821.3973+-10.8685   ?   822.8782+-10.8859      ?
   audio-beat-detection                 196.3768+-1.2337        195.9786+-1.0787       
   audio-dft                            289.4762+-2.0124    ?   291.9755+-3.5931       ?
   audio-fft                            118.2653+-0.4089    ?   118.5623+-0.3563       ?
   audio-oscillator                     305.7648+-1.4518    ?   307.1338+-1.4232       ?
   imaging-darkroom                     297.0695+-8.0269        296.6311+-8.5114       
   imaging-desaturate                   221.4177+-0.3711    ?   221.4266+-0.4326       ?
   imaging-gaussian-blur                459.9391+-2.2574        458.4150+-0.6549       
   json-parse-financial                  63.0999+-0.2543    ^    62.0548+-0.2780       ^ definitely 1.0168x faster
   json-stringify-tinderbox              79.7542+-1.2215    ^    77.9880+-0.3781       ^ definitely 1.0226x faster
   stanford-crypto-aes                   84.5476+-0.4595         83.8717+-0.4741       
   stanford-crypto-ccm                   77.8462+-0.8944         76.5901+-0.7101         might be 1.0164x faster
   stanford-crypto-pbkdf2               186.2330+-0.8928    ?   187.4214+-0.9307       ?
   stanford-crypto-sha256-iterative      89.8800+-0.2906         89.8685+-0.5059       

   <arithmetic> *                       235.0763+-1.0246        235.0568+-1.2835         might be 1.0001x faster
   <geometric>                          176.4131+-0.5488        175.8282+-0.5446         might be 1.0033x faster
   <harmonic>                           138.4484+-0.3933    ^   137.3959+-0.2900       ^ definitely 1.0077x faster

                                             Baseline              StringSplit                                   
All benchmarks:
   <arithmetic>                          88.7200+-0.4915         88.6777+-0.5338         might be 1.0005x faster
   <geometric>                           23.4720+-0.1403         23.4495+-0.1426         might be 1.0010x faster
   <harmonic>                             7.7185+-0.0497    ?     7.7350+-0.0458       ? might be 1.0021x slower

                                             Baseline              StringSplit                                   
Geomean of preferred means:
   <scaled-result>                       53.0219+-0.4465         52.9607+-0.4533         might be 1.0012x faster

[Gustavo Noronha (kov)]

Comment on attachment 138902 [details]
Proposed Patch

Attachment 138902 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12524875

[Early Warning System Bot]

Comment on attachment 138902 [details]
Proposed Patch

Attachment 138902 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12524874

[Build Bot]

Comment on attachment 138902 [details]
Proposed Patch

Attachment 138902 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12525690

[Build Bot]

Comment on attachment 138902 [details]
Proposed Patch

Attachment 138902 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12535412

[Early Warning System Bot]

Comment on attachment 138902 [details]
Proposed Patch

Attachment 138902 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12520724

[Gyuyoung Kim]

Comment on attachment 138902 [details]
Proposed Patch

Attachment 138902 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12518741

[Michael Saboff]

Created attachment 139017 [details]
Patch with Speculative Build Fix

clang doesn't produce the errors.

Added jsString() and jsStringFromArguments() as friends to JSRopeString

[Build Bot]

Comment on attachment 139017 [details]
Patch with Speculative Build Fix

Attachment 139017 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12529895

[Michael Saboff]

Created attachment 139041 [details]
Updated patch with speculative Windows fix

[Build Bot]

Comment on attachment 139041 [details]
Updated patch with speculative Windows fix

Attachment 139041 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12490900

[Michael Saboff]

Created attachment 139058 [details]
Another Windows build fix - added new symbol to exports

[Geoffrey Garen]

Comment on attachment 139058 [details]
Another Windows build fix - added new symbol to exports

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

Looks good, but some small tweaks required before landing.

> Source/JavaScriptCore/runtime/JSString.cpp:63
> +    if (thisObject->isJSRopeStringObject())
> +        JSRopeString::visitChildren(thisObject, visitor);
> +}
> +
> +void JSRopeString::visitChildren(JSString* thisObject, SlotVisitor& visitor)

It's slightly dirty pool to have two classes with distinct GC visit functions but identical ClassInfos. ClassInfo is supposed to contain a pointer to an object's visitChildren function. It's also against our convention to have a subclass "visitChildren" function that never calls its base class's "visitChildren" function.

I think a slightly better way to represent this is to fold the rope-style marking code into JSString::visitChildren. That way, JSString's ClassInfo is still "true", and JSRopeString is just an implementation detail for the fact that a JSString may be variable-sized.

> Source/JavaScriptCore/runtime/JSString.cpp:65
> +    JSRopeString* thisObjectAsRope = reinterpret_cast<JSRopeString*>(thisObject);

static_cast is the C++ cast for casting a base class pointer to a subclass pointer.

> Source/JavaScriptCore/runtime/JSString.cpp:157
> +            JSRopeString* currentFiberAsRope = reinterpret_cast<JSRopeString*>(currentFiber);

static_cast is the C++ cast for casting a base class pointer to a subclass pointer.

> Source/JavaScriptCore/runtime/JSString.cpp:186
> +            JSRopeString* currentFiberAsRope = reinterpret_cast<JSRopeString*>(currentFiber);

static_cast is the C++ cast for casting a base class pointer to a subclass pointer.

> Source/JavaScriptCore/runtime/JSString.h:167
> +        bool isRope() const { return m_value.isNull(); }
> +        bool is8Bit() const { return m_is8Bit; }
> +        bool isJSRopeStringObject() const { return m_isJSRopeStringObject; }

I'd rather not have two different ways to ask "Am I a rope?" -- It's hard to tell which one to use when. Instead, please always use the existing isRope() function, and remove the new one.

> Source/JavaScriptCore/runtime/JSString.h:349
> +            reinterpret_cast<const JSRopeString*>(this)->resolveRope(exec);

static_cast is the C++ cast for casting a base class pointer to a subclass pointer.

> Source/JavaScriptCore/runtime/JSString.h:356
> +            reinterpret_cast<const JSRopeString*>(this)->resolveRope(0);

static_cast is the C++ cast for casting a base class pointer to a subclass pointer.

> Source/JavaScriptCore/runtime/JSString.h:364
> +            return reinterpret_cast<JSRopeString*>(this)->getIndexSlowCase(exec, i);

static_cast is the C++ cast for casting a base class pointer to a subclass pointer.

[Michael Saboff]

Created attachment 139228 [details]
Patch updated from review comments

(In reply to comment #13)
> (From update of attachment 139058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139058&action=review
> 
> Looks good, but some small tweaks required before landing.
> 
> > Source/JavaScriptCore/runtime/JSString.cpp:63
> > +    if (thisObject->isJSRopeStringObject())
> > +        JSRopeString::visitChildren(thisObject, visitor);
> > +}
> > +
> > +void JSRopeString::visitChildren(JSString* thisObject, SlotVisitor& visitor)
> 
> It's slightly dirty pool to have two classes with distinct GC visit functions but identical ClassInfos. ClassInfo is supposed to contain a pointer to an object's visitChildren function. It's also against our convention to have a subclass "visitChildren" function that never calls its base class's "visitChildren" function.
> 
> I think a slightly better way to represent this is to fold the rope-style marking code into JSString::visitChildren. That way, JSString's ClassInfo is still "true", and JSRopeString is just an implementation detail for the fact that a JSString may be variable-sized.

Modified static JSRopeString::visitChildren() to JSRopeString::visitFibers().

> > Source/JavaScriptCore/runtime/JSString.cpp:65
> > +    JSRopeString* thisObjectAsRope = reinterpret_cast<JSRopeString*>(thisObject);
> 
> static_cast is the C++ cast for casting a base class pointer to a subclass pointer.

Fixed all cited reinterpret_casts to static_casts.

> > Source/JavaScriptCore/runtime/JSString.h:167
> > +        bool isRope() const { return m_value.isNull(); }
> > +        bool is8Bit() const { return m_is8Bit; }
> > +        bool isJSRopeStringObject() const { return m_isJSRopeStringObject; }
> 
> I'd rather not have two different ways to ask "Am I a rope?" -- It's hard to tell which one to use when. Instead, please always use the existing isRope() function, and remove the new one.

They actually serve two different purposes.  isRope() is really isUnresolvedRope() so I changed it's name.  When a JSRopeString is created, isRope() (now isUnresolvedRope()) is true, but when the rope is resolved, it becomes false.  isJSRopeStringObject() will be true throughout the life of the JSRopeObject and is used only in JSString::visitChildren() to make sure we visit the fibers.

[Michael Saboff]

Created attachment 139233 [details]
Updated patch after talking to Geoff about isRope / isJSRopeObject()

Eliminated isJSRopeObject() and reverted isUnresolvedRope() to isRope().

[Geoffrey Garen]

Comment on attachment 139233 [details]
Updated patch after talking to Geoff about isRope / isJSRopeObject()

r=me

[Michael Saboff]

Committed r115516: <http://trac.webkit.org/changeset/115516>