Bug 67072 – JSC::Executable is inconsistent about using weak handle finalizers and destructors for releasing memory

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

For executable code, JSC::Executable uses a weak handle finalizer.  This ensures the executable code is released as soon as the GC knows that the Executable object is no longer reachable.  For everything else, JSC::Executable uses a destructor, which may only run when the cell in which JSC::Executable was allocated gets used for a new allocation.

This is not a bad approach from a performance standpoint, since releasing executable code as soon as possible is always a good policy.  But, the CodeBlock object is itself sizable, so releasing it sooner would be nice for memory usage.  It's also true that moving all memory release into a finalizer is a good step towards ultimately not executing destructors.  Doing so also makes confusing code slightly less confusing.  The code is necessarily confusing because certain state in CodeBlock is stored in Executable to make it quicker to query during function dispatch.  The current approach means that the part of CodeBlock's state that is stored in Executable is freed early, so when the CodeBlock is freed later from the destructor, some of its state is already released.  It's probably wise to continue to store (or at least cache) some of CodeBlock's state in Executable, but it would make things somewhat easier to understand if the release of CodeBlock memory happened at the same time as the release of CodeBlock state that was cached in Executable.

[Filip Pizlo]

Created attachment 105424 [details]
the patch

Tests seem to pass, it seems that with this patch I can still browse the web, and performance appears neutral.  Full performance report below.


Benchmark report for SunSpider, V8, and Kraken.

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc
"ExecFinalize" at /Volumes/Data/pizlo/quartary/OpenSource/WebKitBuild/Release/jsc

Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. 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.

                                            TipOfTree              ExecFinalize                                  
SunSpider:
   3d-cube                                7.6708+-0.1176          7.5841+-0.1639         might be 1.0114x faster
   3d-morph                               7.5792+-0.1602          7.3793+-0.1658         might be 1.0271x faster
   3d-raytrace                            7.6858+-0.1833          7.6331+-0.1749       
   access-binary-trees                    2.3464+-0.1027          2.2665+-0.0545         might be 1.0353x faster
   access-fannkuch                       11.6685+-0.2678    ?    11.7823+-0.2536       ?
   access-nbody                           4.2522+-0.0905          4.1475+-0.0589         might be 1.0252x faster
   access-nsieve                          2.5671+-0.0849          2.4992+-0.0802         might be 1.0272x faster
   bitops-3bit-bits-in-byte               1.7282+-0.0525    ?     1.7325+-0.0437       ?
   bitops-bits-in-byte                    4.4494+-0.2025          4.4308+-0.1947       
   bitops-bitwise-and                     3.6244+-0.0621    ?     3.6801+-0.0639       ? might be 1.0154x slower
   bitops-nsieve-bits                     5.4832+-0.1309          5.3283+-0.1084         might be 1.0291x faster
   controlflow-recursive                  2.0154+-0.0503    ?     2.1204+-0.0739       ? might be 1.0521x slower
   crypto-aes                             6.4391+-0.2795    ?     6.6213+-0.1974       ? might be 1.0283x slower
   crypto-md5                             2.8146+-0.0728          2.7349+-0.0893         might be 1.0291x faster
   crypto-sha1                            2.2402+-0.0931          2.2180+-0.0429         might be 1.0100x faster
   date-format-tofte                     10.1556+-0.1648         10.0386+-0.1697         might be 1.0117x faster
   date-format-xparb                      8.6652+-0.2455          8.3647+-0.2152         might be 1.0359x faster
   math-cordic                            6.2553+-0.1021    ?     6.4305+-0.1790       ? might be 1.0280x slower
   math-partial-sums                      7.6624+-0.1552          7.5720+-0.1294         might be 1.0119x faster
   math-spectral-norm                     2.4831+-0.0550          2.4610+-0.0428       
   regexp-dna                            10.2313+-0.1627    ?    10.3778+-0.2369       ? might be 1.0143x slower
   string-base64                          6.0103+-0.1399    ?     6.0212+-0.1152       ?
   string-fasta                           7.3684+-0.1139          7.3548+-0.1170       
   string-tagcloud                       13.3408+-0.2701    ?    13.3510+-0.2191       ?
   string-unpack-code                    18.5239+-0.3307         18.4312+-0.2688       
   string-validate-input                  7.3392+-0.3738          7.2632+-0.1464         might be 1.0105x faster

   <arithmetic>                           6.5615+-0.0347          6.5317+-0.0292       
   <geometric>                            5.4509+-0.0349          5.4234+-0.0285       
   <harmonic>                             4.4624+-0.0390          4.4418+-0.0268       

                                            TipOfTree              ExecFinalize                                  
V8:
   crypto                                89.9669+-0.9327         89.8237+-0.5736       
   deltablue                            265.7069+-2.3741        263.3825+-1.1593       
   earley-boyer                         101.6921+-0.7357        101.3654+-0.7025       
   raytrace                              79.4849+-2.3809         77.1534+-0.4093         might be 1.0302x faster
   regexp                               109.1505+-0.7869    ?   110.0358+-1.0691       ?
   richards                             240.8500+-1.5259    ?   242.0161+-1.1816       ?
   splay                                108.6089+-0.7801    ?   108.7020+-1.0020       ?

   <arithmetic>                         142.2086+-0.6081        141.7827+-0.3095       
   <geometric>                          127.6110+-0.7872        127.0863+-0.3010       
   <harmonic>                           116.9864+-0.9733        116.3146+-0.3005       

                                            TipOfTree              ExecFinalize                                  
Kraken:
   ai-astar                            1089.1605+-11.5608   ?  1093.1558+-10.3830      ?
   audio-beat-detection                 469.1552+-2.4998    ?   472.8775+-4.3576       ?
   audio-dft                            423.7785+-12.0481   ?   427.6782+-12.8277      ?
   audio-fft                            366.1977+-0.7915    !   369.2613+-0.7147       ! definitely 1.0084x slower
   audio-oscillator                     399.8224+-4.1327        397.2567+-2.8732       
   imaging-darkroom                     526.6979+-3.3260    ?   530.2128+-3.4240       ?
   imaging-desaturate                   586.1033+-6.2498    ?   588.2981+-7.4616       ?
   imaging-gaussian-blur               1704.6206+-4.4376    ?  1705.4177+-5.6552       ?
   json-parse-financial                  48.0213+-0.2319    !    48.6721+-0.3170       ! definitely 1.0136x slower
   json-stringify-tinderbox              62.1680+-0.8153    ?    62.6272+-0.6596       ?
   stanford-crypto-aes                  143.6970+-1.8040    ?   144.7303+-1.9498       ?
   stanford-crypto-ccm                  111.4988+-1.1214        111.4743+-0.7385       
   stanford-crypto-pbkdf2               335.6885+-2.1689    ?   336.1219+-1.8932       ?
   stanford-crypto-sha256-iterative     129.5380+-0.3716    ?   132.1052+-3.1976       ? might be 1.0198x slower

   <arithmetic>                         456.8677+-1.9499    ?   458.5635+-1.2238       ?
   <geometric>                          293.1670+-1.1406    ?   294.8764+-0.9579       ?
   <harmonic>                           178.6056+-0.5588    !   180.1183+-0.7114       ! definitely 1.0085x slower

                                            TipOfTree              ExecFinalize                                  
All benchmarks:
   <arithmetic>                         160.8980+-0.6071    ?   161.3232+-0.3711       ?
   <geometric>                           28.5710+-0.1106         28.5233+-0.0851       
   <harmonic>                             7.8796+-0.0674          7.8446+-0.0462

[Darin Adler]

Comment on attachment 105424 [details]
the patch

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

> Source/JavaScriptCore/runtime/Executable.cpp:251
> +    if (m_evalCodeBlock)
> +        m_evalCodeBlock->clearEvalCache();
> +    m_evalCodeBlock.clear();

Could put the clear inside the if.

> Source/JavaScriptCore/runtime/Executable.cpp:343
> +    if (m_programCodeBlock)
> +        m_programCodeBlock->clearEvalCache();
> +    m_programCodeBlock.clear();

Could put the clear inside the if.

> Source/JavaScriptCore/runtime/Executable.cpp:496
>      if (m_codeBlockForCall)
>          m_codeBlockForCall->clearEvalCache();
>      m_codeBlockForCall.clear();

Could put the clear inside the if.

> Source/JavaScriptCore/runtime/Executable.cpp:499
>      if (m_codeBlockForConstruct)
>          m_codeBlockForConstruct->clearEvalCache();
>      m_codeBlockForConstruct.clear();

Could put the clear inside the if.

> Source/JavaScriptCore/runtime/Executable.h:-69
> -#if ENABLE(JIT)
>              Weak<ExecutableBase> finalizer(globalData, this, executableFinalizer());
>              finalizer.leakHandle();
> -#endif

Would be nice to have a comment in the change log explaining this change.

> Source/JavaScriptCore/runtime/Executable.h:337
> +        virtual void clearCode();

I like to follow a pattern where virtual functions overrides are private or protected, even when the base class function is public. This sometimes lets us discover cases where we would be doing a virtual function dispatch and could instead do a non-virtual, and has little cost.

Accordingly, I suggest making this protected and private in derived classes, and leave it public only in ExecutableBase.

[Filip Pizlo]

Created attachment 105442 [details]
the patch (fix review)

[WebKit Review Bot]

Comment on attachment 105442 [details]
the patch (fix review)

Clearing flags on attachment: 105442

Committed r93947: <http://trac.webkit.org/changeset/93947>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.