Bug 141275 – DelayedReleaseScope drops locks during GC which can cause a thread switch and code reentry

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

In CodeCache::getGlobalCodeBlock(), the CodeCacheMap::add() method will initially add a null UnlinkedCodeBlockType* and then set the value to the new code block after it is created.  The creation of that code block could block, drop all of its locks and allow another thread to begin executing.  That second thread could also add the same source to the CadeCache and this time it finds the entry the first thread created which has a null UnlinkedCodeBlockType*.  This second thread will try to use that null pointer and then crash.

Here is a sample stack trace for the first thread:
* thread #18: tid = 0x8756a0, 0x000000010479c076 libsystem_kernel.dylib`__psynch_mutexwait + 10, queue = 'JSTEval'
  * frame #0: 0x000000010479c076 libsystem_kernel.dylib`__psynch_mutexwait + 10
    frame #1: 0x00000001048ca744 libsystem_pthread.dylib`_pthread_mutex_lock + 482
    frame #2: 0x0000000104391f67 libc++.1.dylib`std::__1::mutex::lock() + 9
    frame #3: 0x000000010069353d JavaScriptCore`JSC::JSLock::lock(this=0x0000000119ffb000, lockCount=2) + 157 at JSLock.cpp:121
    frame #4: 0x0000000100693cea JavaScriptCore`JSC::JSLock::grabAllLocks(this=0x0000000119ffb000, dropper=0x000000011b7358c8, droppedLockCount=2) + 186 at JSLock.cpp:237
    frame #5: 0x000000010069407d JavaScriptCore`JSC::JSLock::DropAllLocks::~DropAllLocks(this=0x000000011b7358c8) + 109 at JSLock.cpp:280
    frame #6: 0x0000000100694105 JavaScriptCore`JSC::JSLock::DropAllLocks::~DropAllLocks(this=0x000000011b7358c8) + 21 at JSLock.cpp:277
    frame #7: 0x000000010051ce84 JavaScriptCore`JSC::DelayedReleaseScope::~DelayedReleaseScope(this=0x000000011b7359e0) + 260 at DelayedReleaseScope.h:57
    frame #8: 0x0000000100512615 JavaScriptCore`JSC::DelayedReleaseScope::~DelayedReleaseScope(this=0x000000011b7359e0) + 21 at DelayedReleaseScope.h:47
    frame #9: 0x0000000100773de0 JavaScriptCore`JSC::MarkedAllocator::tryAllocateHelper(this=0x0000000118013e78, bytes=336) + 672 at MarkedAllocator.cpp:105
    frame #10: 0x0000000100772212 JavaScriptCore`JSC::MarkedAllocator::tryAllocate(this=0x0000000118013e78, bytes=336) + 114 at MarkedAllocator.cpp:129
    frame #11: 0x0000000100771afe JavaScriptCore`JSC::MarkedAllocator::allocateSlowCase(this=0x0000000118013e78, bytes=336) + 254 at MarkedAllocator.cpp:171
    frame #12: 0x000000010003c8a1 JavaScriptCore`JSC::MarkedAllocator::allocate(this=0x0000000118013e78, bytes=336) + 81 at MarkedAllocator.h:95
    frame #13: 0x000000010004da59 JavaScriptCore`JSC::MarkedSpace::allocateWithImmortalStructureDestructor(this=0x0000000118010328, bytes=336) + 41 at MarkedSpace.h:251
    frame #14: 0x000000010004da26 JavaScriptCore`JSC::Heap::allocateWithImmortalStructureDestructor(this=0x0000000118010018, bytes=336) + 118 at HeapInlines.h:196
    frame #15: 0x0000000100122ac7 JavaScriptCore`void* JSC::allocateCell<JSC::UnlinkedProgramCodeBlock>(heap=0x0000000118010018, size=336) + 151 at JSCellInlines.h:134
    frame #16: 0x000000010012284c JavaScriptCore`void* JSC::allocateCell<JSC::UnlinkedProgramCodeBlock>(heap=0x0000000118010018) + 28 at JSCellInlines.h:150
    frame #17: 0x00000001001225e3 JavaScriptCore`JSC::UnlinkedProgramCodeBlock::create(vm=0x0000000118010000, info=0x000000011b735d48) + 35 at UnlinkedCodeBlock.h:622
    frame #18: 0x000000010011daa0 JavaScriptCore`JSC::UnlinkedProgramCodeBlock* JSC::CodeCache::getGlobalCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(this=0x0000000119ff7058, vm=0x0000000118010000, executable=0x000000011a33fc70, source=0x000000011a33fcb0, strictness=JSParseNormal, debuggerMode=DebuggerOff, profilerMode=ProfilerOff, error=0x000000011b736230) + 1856 at CodeCache.cpp:107
    frame #19: 0x000000010011c9de JavaScriptCore`JSC::CodeCache::getProgramCodeBlock(this=0x0000000119ff7058, vm=0x0000000118010000, executable=0x000000011a33fc70, source=0x000000011a33fcb0, strictness=JSParseNormal, debuggerMode=DebuggerOff, profilerMode=ProfilerOff, error=0x000000011b736230) + 94 at CodeCache.cpp:128
    frame #20: 0x000000010065da9b JavaScriptCore`JSC::JSGlobalObject::createProgramCodeBlock(this=0x000000011a2df970, callFrame=0x000000011a2df9b0, executable=0x000000011a33fc70, exception=0x000000011b736388) + 315 at JSGlobalObject.cpp:765
    frame #21: 0x000000010043ad9d JavaScriptCore`JSC::ProgramExecutable::initializeGlobalProperties(this=0x000000011a33fc70, vm=0x0000000118010000, callFrame=0x000000011a2df9b0, scope=0x000000011a2df970) + 253 at Executable.cpp:489
    frame #22: 0x00000001005c37f3 JavaScriptCore`JSC::Interpreter::execute(this=0x0000000119ff4000, program=0x000000011a33fc70, callFrame=0x000000011a2df9b0, thisObj=0x000000011a2efaf0) + 3795 at Interpreter.cpp:889
    frame #23: 0x00000001001448e0 JavaScriptCore`JSC::evaluate(exec=0x000000011a2df9b0, source=0x000000011b737b10, thisValue=JSValue at 0x000000011b737a70, returnedException=0x000000011b737ae0) + 480 at Completion.cpp:81
    frame #24: 0x000000010062cfea JavaScriptCore`JSEvaluateScript(ctx=0x000000011a2df9b0, script=0x0000000119fd03c0, thisObject=0x0000000000000000, sourceURL=0x0000000000000000, startingLineNumber=1, exception=0x000000011b737c00) + 538 at JSBase.cpp:66
    frame #25: 0x000000010064562e JavaScriptCore`-[JSContext evaluateScript:withSourceURL:](self=0x0000628000043210, _cmd=0x0000000100c914fc, script=0x0000000100008760, sourceURL=0x0000000000000000) + 158 at JSContext.mm:102
    frame #26: 0x0000000100645582 JavaScriptCore`-[JSContext evaluateScript:](self=0x0000628000043210, _cmd=0x0000000100006f27, script=0x0000000100008760) + 66 at JSContext.mm:94
    frame #27: 0x0000000100002f17 JSCTester`__42-[JSTEvaluator evaluateScript:completion:]_block_invoke(.block_descriptor=0x0000628000041230, context=0x0000628000043210) + 71 at JSTEvaluator.m:172
    frame #28: 0x0000000100004c0c JSCTester`__30-[JSTEvaluator _sourcePerform]_block_invoke158(.block_descriptor=<unavailable>) + 220 at JSTEvaluator.m:343
    frame #29: 0x000000010459cc64 libdispatch.dylib`_dispatch_call_block_and_release + 12
    frame #30: 0x00000001045976ec libdispatch.dylib`_dispatch_client_callout + 8
    frame #31: 0x00000001045ae4cf libdispatch.dylib`_dispatch_async_redirect_invoke + 2746
    frame #32: 0x00000001045976ec libdispatch.dylib`_dispatch_client_callout + 8
    frame #33: 0x000000010459b9a4 libdispatch.dylib`_dispatch_root_queue_drain + 2858
    frame #34: 0x000000010459ae73 libdispatch.dylib`_dispatch_worker_thread3 + 106
    frame #35: 0x00000001048ccb1d libsystem_pthread.dylib`_pthread_wqthread + 729
    frame #36: 0x00000001048ca489 libsystem_pthread.dylib`start_wqthread + 13

Here is the stack trace for the second thread:
* thread #6: tid = 0x875633, 0x000000010011f43c JavaScriptCore`JSC::UnlinkedCodeBlock::firstLine(this=0x0000000000000000) const + 12 at UnlinkedCodeBlock.h:485, queue = 'JSTEval', stop reason = EXC_BAD_ACCESS (code=1, address=0x48)
  * frame #0: 0x000000010011f43c JavaScriptCore`JSC::UnlinkedCodeBlock::firstLine(this=0x0000000000000000) const + 12 at UnlinkedCodeBlock.h:485
    frame #1: 0x000000010011d538 JavaScriptCore`JSC::UnlinkedProgramCodeBlock* JSC::CodeCache::getGlobalCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(this=0x0000000119ff7058, vm=0x0000000118010000, executable=0x000000011a33fb70, source=0x000000011a33fbb0, strictness=JSParseNormal, debuggerMode=DebuggerOff, profilerMode=ProfilerOff, error=0x000000010e102230) + 472 at CodeCache.cpp:85
    frame #2: 0x000000010011c9de JavaScriptCore`JSC::CodeCache::getProgramCodeBlock(this=0x0000000119ff7058, vm=0x0000000118010000, executable=0x000000011a33fb70, source=0x000000011a33fbb0, strictness=JSParseNormal, debuggerMode=DebuggerOff, profilerMode=ProfilerOff, error=0x000000010e102230) + 94 at CodeCache.cpp:128
    frame #3: 0x000000010065da9b JavaScriptCore`JSC::JSGlobalObject::createProgramCodeBlock(this=0x000000011a2df970, callFrame=0x000000011a2df9b0, executable=0x000000011a33fb70, exception=0x000000010e102388) + 315 at JSGlobalObject.cpp:765
    frame #4: 0x000000010043ad9d JavaScriptCore`JSC::ProgramExecutable::initializeGlobalProperties(this=0x000000011a33fb70, vm=0x0000000118010000, callFrame=0x000000011a2df9b0, scope=0x000000011a2df970) + 253 at Executable.cpp:489
    frame #5: 0x00000001005c37f3 JavaScriptCore`JSC::Interpreter::execute(this=0x0000000119ff4000, program=0x000000011a33fb70, callFrame=0x000000011a2df9b0, thisObj=0x000000011a2efaf0) + 3795 at Interpreter.cpp:889
    frame #6: 0x00000001001448e0 JavaScriptCore`JSC::evaluate(exec=0x000000011a2df9b0, source=0x000000010e103b10, thisValue=JSValue at 0x000000010e103a70, returnedException=0x000000010e103ae0) + 480 at Completion.cpp:81
    frame #7: 0x000000010062cfea JavaScriptCore`JSEvaluateScript(ctx=0x000000011a2df9b0, script=0x0000000119fe6060, thisObject=0x0000000000000000, sourceURL=0x0000000000000000, startingLineNumber=1, exception=0x000000010e103c00) + 538 at JSBase.cpp:66
    frame #8: 0x000000010064562e JavaScriptCore`-[JSContext evaluateScript:withSourceURL:](self=0x0000628000043210, _cmd=0x0000000100c914fc, script=0x0000000100008760, sourceURL=0x0000000000000000) + 158 at JSContext.mm:102
    frame #9: 0x0000000100645582 JavaScriptCore`-[JSContext evaluateScript:](self=0x0000628000043210, _cmd=0x0000000100006f27, script=0x0000000100008760) + 66 at JSContext.mm:94
    frame #10: 0x0000000100002f17 JSCTester`__42-[JSTEvaluator evaluateScript:completion:]_block_invoke(.block_descriptor=0x0000628000041440, context=0x0000628000043210) + 71 at JSTEvaluator.m:172
    frame #11: 0x0000000100004c0c JSCTester`__30-[JSTEvaluator _sourcePerform]_block_invoke158(.block_descriptor=<unavailable>) + 220 at JSTEvaluator.m:343
    frame #12: 0x000000010459cc64 libdispatch.dylib`_dispatch_call_block_and_release + 12
    frame #13: 0x00000001045976ec libdispatch.dylib`_dispatch_client_callout + 8
    frame #14: 0x00000001045ae4cf libdispatch.dylib`_dispatch_async_redirect_invoke + 2746
    frame #15: 0x00000001045976ec libdispatch.dylib`_dispatch_client_callout + 8
    frame #16: 0x000000010459b9a4 libdispatch.dylib`_dispatch_root_queue_drain + 2858
    frame #17: 0x000000010459ae73 libdispatch.dylib`_dispatch_worker_thread3 + 106
    frame #18: 0x00000001048ccb1d libsystem_pthread.dylib`_pthread_wqthread + 729
    frame #19: 0x00000001048ca489 libsystem_pthread.dylib`start_wqthread + 13

This was found with an internal multi-threaded test app.

rdar://problem/19708290

[Michael Saboff]

Created attachment 246072 [details]
Patch

[Mark Lam]

Comment on attachment 246072 [details]
Patch

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

I've provided some feedback for now but I'm not sold on this solution.  The main issue I see is that we used to release all the delayed releases at the end of each GC cycle.  Now, with this patch, we'll have to wait till the outermost JSLock is unlocked.  If a piece of JS code is long running (e.g. the benchmarks), it will continue to retain these DelayedReleasedObjects for a long time even if we've GC'ed several times.  As a result, the runtime memory profile will higher peak usage.

From your bug description, I think the issue at hand is that another thread can see an null UnlinkedCodeBlock in the Cache.  The fact that we drop all locks may have enabled the issue to manifest, but in my understanding is not core to the issue.  Instead, would be possible to fix the issue by changing CodeCache to:

1. Instead of add the SourceCode immediately, do a find to see if an UnlinkedCodeBlock for it exists.
2. If it exists, use the cached UnlinkedCodeBlock.
3. If not:
   3.1 go ahead and parse the SourceCode and make the UnlinkedCodeBlock.
   3.2. Check again is an UnlinkedCodeBlock has already been added for this SourceCode.
   3.3. If not already present, add the UnlinkedCodeBlock to the cache.
   3.4. If already present, release the new UnlinkedCodeBlock, and use the one in the cache.

Theoretically, this might result in some wasted work where the same SourceCode being parsed into an UnlinkedCodeBlock twice.  However, in practice, if we don't have 2 threads, that will never happen.  Even with running on 2 threads, the collision mwill probably still be rare.

Would that work?

> Source/JavaScriptCore/ChangeLog:7
> +

Your ChangeLog comment describes what was done but not why.  I think your description of the issue in bugzilla did a good job of explaining what the issue is.  Would you mind adding it here before you start explaining how it is fixed?

> Source/JavaScriptCore/ChangeLog:17
> +        by calling into JS.  This case is acually tested by testapi.mm.

typo: "acually".  However, I think "already" would be a better fit here.

> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:1758
> -</Project>
>  \ No newline at end of file
> +</Project>

Is this necessary and ok to do?

> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:4390
> -</Project>
>  \ No newline at end of file
> +</Project>

Ditto.

> Source/JavaScriptCore/heap/Heap.h:118
> +    JS_EXPORT_PRIVATE void releaseDelayedReleasedObjects();

No need to JS_EXPORT_PRIVATE this function as it is only used inside JSC.

[Michael Saboff]

(In reply to comment #2)
> Comment on attachment 246072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246072&action=review
> 
> I've provided some feedback for now but I'm not sold on this solution.  The
> main issue I see is that we used to release all the delayed releases at the
> end of each GC cycle.  Now, with this patch, we'll have to wait till the
> outermost JSLock is unlocked.  If a piece of JS code is long running (e.g.
> the benchmarks), it will continue to retain these DelayedReleasedObjects for
> a long time even if we've GC'ed several times.  As a result, the runtime
> memory profile will higher peak usage.

I don't think any of our benchmarks use the ObjC API or retain ObjC objects.

> From your bug description, I think the issue at hand is that another thread
> can see an null UnlinkedCodeBlock in the Cache.  The fact that we drop all
> locks may have enabled the issue to manifest, but in my understanding is not
> core to the issue.  Instead, would be possible to fix the issue by changing
> CodeCache to:
> 
> 1. Instead of add the SourceCode immediately, do a find to see if an
> UnlinkedCodeBlock for it exists.
> 2. If it exists, use the cached UnlinkedCodeBlock.
> 3. If not:
>    3.1 go ahead and parse the SourceCode and make the UnlinkedCodeBlock.
>    3.2. Check again is an UnlinkedCodeBlock has already been added for this
> SourceCode.
>    3.3. If not already present, add the UnlinkedCodeBlock to the cache.
>    3.4. If already present, release the new UnlinkedCodeBlock, and use the
> one in the cache.
> 
> Theoretically, this might result in some wasted work where the same
> SourceCode being parsed into an UnlinkedCodeBlock twice.  However, in
> practice, if we don't have 2 threads, that will never happen.  Even with
> running on 2 threads, the collision mwill probably still be rare.
> 
> Would that work?

I implemented a trial solution in CodeCache:add() similar to what you describe and it worked fine for this issue.  I discussed it with Geoff and he said the problem with that solution is there are many other places that we do this "add null and fill in the value later" kind of HashMap accesses.  From that discussion, we agreed on this approach of not dropping the lock during GC and releasing delayed objects after the lock was dropped exiting the VM.

> > Source/JavaScriptCore/ChangeLog:7
> > +
> 
> Your ChangeLog comment describes what was done but not why.  I think your
> description of the issue in bugzilla did a good job of explaining what the
> issue is.  Would you mind adding it here before you start explaining how it
> is fixed?

I added this paragraph to the top of the ChangeLog entry:
    The root issue is that one thread, while allocating JS objects may need to garbage collect.
    During that garbage collection, if there are delayed Objective C objects that need to be
    released, we drop the locks to release them.  While the locks are release by that thread,
    another thread can enter the VM and might have exactly the same source and therefore hit
    this issue.  This fixes the problem by not dropping the locks during garbage collection.

> > Source/JavaScriptCore/ChangeLog:17
> > +        by calling into JS.  This case is acually tested by testapi.mm.
> 
> typo: "acually".  However, I think "already" would be a better fit here.

Made that fix locally.

> > Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:1758
> > -</Project>
> >  \ No newline at end of file
> > +</Project>
> 
> Is this necessary and ok to do?

Given that it is XML, I think it is fine.  The Win EWS bot built with the patch just fine.
 
> > Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:4390
> > -</Project>
> >  \ No newline at end of file
> > +</Project>
> 
> Ditto.
> 
> > Source/JavaScriptCore/heap/Heap.h:118
> > +    JS_EXPORT_PRIVATE void releaseDelayedReleasedObjects();
> 
> No need to JS_EXPORT_PRIVATE this function as it is only used inside JSC.

Fixed.

[Michael Saboff]

Here are benchmark results.  Neutral to maybe a slight speed up.

Benchmark report for SunSpider, LongSpider, V8Spider, Octane, Kraken, JSRegress, AsmBench, and CompressionBench on msaboff-pro (MacPro5,1).

VMs tested:
"Baseline" at /Volumes/Data/src/webkit.baseline/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc
"GCDontDropLocks" at /Volumes/Data/src/webkit/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc

Collected 4 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              GCDontDropLocks                                  
SunSpider:
   3d-cube                                            8.2874+-0.4075     ?      8.4276+-0.1794        ? might be 1.0169x slower
   3d-morph                                          12.6283+-0.2559           12.6267+-0.3875        
   3d-raytrace                                       11.0775+-0.3408           10.9921+-0.3171        
   access-binary-trees                                3.5512+-0.2837            3.4613+-0.2011          might be 1.0260x faster
   access-fannkuch                                   10.4371+-0.3147     ?     10.5487+-0.1167        ? might be 1.0107x slower
   access-nbody                                       5.6537+-0.0808            5.6317+-0.2477        
   access-nsieve                                      5.7880+-0.2364     ?      6.1447+-0.7727        ? might be 1.0616x slower
   bitops-3bit-bits-in-byte                           2.3798+-0.1259            2.3016+-0.1406          might be 1.0340x faster
   bitops-bits-in-byte                                6.4005+-0.1407     ?      6.6443+-0.5729        ? might be 1.0381x slower
   bitops-bitwise-and                                 3.1192+-0.1480     ?      3.1356+-0.0724        ?
   bitops-nsieve-bits                                 6.8399+-0.3972     ?      6.8992+-0.0346        ?
   controlflow-recursive                              3.6331+-0.2296     ?      3.6355+-0.3157        ?
   crypto-aes                                         7.1508+-0.4122     ?      7.1888+-0.4024        ?
   crypto-md5                                         4.3215+-0.2786            4.3129+-0.5220        
   crypto-sha1                                        4.3408+-0.1204            4.1880+-0.2596          might be 1.0365x faster
   date-format-tofte                                 16.7717+-0.7366           16.6817+-0.2536        
   date-format-xparb                                  9.4168+-0.5794     ?      9.5230+-0.4646        ? might be 1.0113x slower
   math-cordic                                        5.6259+-0.2124            5.6171+-0.3203        
   math-partial-sums                                 11.0132+-0.1232     ?     11.0358+-0.2127        ?
   math-spectral-norm                                 3.6334+-0.1861            3.5872+-0.0561          might be 1.0129x faster
   regexp-dna                                        11.6884+-0.5060           11.3149+-0.2350          might be 1.0330x faster
   string-base64                                      7.2542+-0.3255            7.2304+-0.3460        
   string-fasta                                      12.2562+-0.3317     ?     12.3046+-0.4177        ?
   string-tagcloud                                   17.7007+-0.3883           17.3237+-0.3029          might be 1.0218x faster
   string-unpack-code                                37.3306+-1.0074     ?     38.2923+-1.0488        ? might be 1.0258x slower
   string-validate-input                              8.5934+-0.3738            8.4857+-0.1985          might be 1.0127x faster

   <arithmetic> *                                     9.1113+-0.0862     ?      9.1360+-0.1193        ? might be 1.0027x slower
   <geometric>                                        7.4436+-0.0882            7.4396+-0.1216          might be 1.0005x faster
   <harmonic>                                         6.2588+-0.1045            6.2312+-0.1153          might be 1.0044x faster

                                                         Baseline              GCDontDropLocks                                  
LongSpider:
   3d-cube                                         1600.9268+-29.6274        1597.9611+-28.7411       
   3d-morph                                        1980.2997+-4.5007         1979.3760+-1.9151        
   3d-raytrace                                     1563.5179+-9.3382         1561.7790+-9.3141        
   access-binary-trees                             1870.8630+-11.6636        1853.6997+-8.9547        
   access-fannkuch                                  598.4971+-44.1336    ?    600.7025+-29.9472       ?
   access-nbody                                    1486.9128+-8.4505     ?   1490.3475+-11.1716       ?
   access-nsieve                                   1814.2460+-11.8846    ?   1819.5962+-20.7864       ?
   bitops-3bit-bits-in-byte                          71.4328+-0.9201           71.0125+-1.0927        
   bitops-bits-in-byte                              369.8054+-11.8523    ?    373.0720+-14.0135       ?
   bitops-nsieve-bits                              1453.0237+-16.2679    ?   1453.2216+-11.1583       ?
   controlflow-recursive                           1089.6410+-3.6694     ?   1099.3997+-8.3668        ?
   crypto-aes                                      1297.5742+-8.4535         1293.8137+-5.1115        
   crypto-md5                                      1083.3359+-11.6202        1082.2216+-9.4268        
   crypto-sha1                                     1449.4463+-2.9528     ?   1450.9452+-9.7966        ?
   date-format-tofte                               1514.6633+-84.9346    ?   1526.6085+-25.2648       ?
   date-format-xparb                               1423.4734+-27.9681        1388.8990+-28.5513         might be 1.0249x faster
   math-cordic                                     1051.2505+-12.7771        1048.5900+-9.7412        
   math-partial-sums                               1285.7495+-2.8893         1285.5657+-5.8522        
   math-spectral-norm                              1218.3607+-1.6995     ?   1218.5185+-2.5982        ?
   string-base64                                    635.0610+-1.5039          634.2584+-2.8186        
   string-fasta                                     873.4131+-26.1124         854.9326+-9.9061          might be 1.0216x faster
   string-tagcloud                                  419.6511+-2.9527          417.9655+-4.1416        

   <arithmetic>                                    1188.6884+-7.9262         1186.4767+-3.6740          might be 1.0019x faster
   <geometric> *                                   1002.3436+-7.7248         1000.5258+-2.7055          might be 1.0018x faster
   <harmonic>                                       630.1777+-5.3515          628.3575+-3.6252          might be 1.0029x faster

                                                         Baseline              GCDontDropLocks                                  
V8Spider:
   crypto                                           100.2801+-1.2653     ?    100.5043+-1.1308        ?
   deltablue                                        126.9694+-5.3371          121.9311+-2.6027          might be 1.0413x faster
   earley-boyer                                      83.0057+-5.4756           80.6595+-0.2354          might be 1.0291x faster
   raytrace                                          51.3168+-0.9058           50.6696+-1.0394          might be 1.0128x faster
   regexp                                           128.9222+-1.4115          128.8653+-0.9100        
   richards                                         132.2358+-2.4264     ?    133.1490+-1.0884        ?
   splay                                             59.2132+-1.3879     ?     61.7690+-6.8158        ? might be 1.0432x slower

   <arithmetic>                                      97.4205+-0.5011           96.7925+-0.9790          might be 1.0065x faster
   <geometric> *                                     91.7400+-0.5463           91.3272+-1.5237          might be 1.0045x faster
   <harmonic>                                        85.7713+-0.7367           85.5814+-2.1441          might be 1.0022x faster

                                                         Baseline              GCDontDropLocks                                  
Octane:
   encrypt                                           0.46692+-0.01103          0.46354+-0.01443       
   decrypt                                           8.24243+-0.03055          8.23960+-0.01946       
   deltablue                                x2       0.39210+-0.00701    ?     0.39272+-0.00444       ?
   earley                                            1.34903+-0.01789          1.33857+-0.03040       
   boyer                                            12.15227+-0.23771         11.99703+-0.02912         might be 1.0129x faster
   navier-stokes                            x2       7.37743+-0.00696    ?     7.38730+-0.02524       ?
   raytrace                                 x2       2.67632+-0.33980          2.65387+-0.20697       
   richards                                 x2       0.24530+-0.00471          0.24205+-0.00553         might be 1.0134x faster
   splay                                    x2       0.66180+-0.02769          0.65440+-0.00381         might be 1.0113x faster
   regexp                                   x2      63.30892+-0.65007         63.24368+-2.23599       
   pdfjs                                    x2     101.20690+-1.02071        100.00601+-1.28065         might be 1.0120x faster
   mandreel                                 x2     106.90159+-1.56042    ?   107.15199+-0.70171       ?
   gbemu                                    x2     100.00511+-4.51717         99.72006+-3.37979       
   closure                                           0.98703+-0.01117    ?     0.98851+-0.01114       ?
   jquery                                           12.52820+-0.14516    ?    12.63393+-0.15504       ?
   box2d                                    x2      28.81709+-0.42253    ^    28.35351+-0.02981       ^ definitely 1.0163x faster
   zlib                                     x2     874.67743+-78.77564   ?   900.54356+-5.96773       ? might be 1.0296x slower
   typescript                               x2    1473.44904+-44.86878   ?  1497.86053+-10.57159      ? might be 1.0166x slower

   <arithmetic>                                    185.17213+-8.15047    ?   188.40269+-0.82534       ? might be 1.0174x slower
   <geometric> *                                    13.48771+-0.19672         13.46913+-0.08889         might be 1.0014x faster
   <harmonic>                                        1.38607+-0.00685          1.37592+-0.01121         might be 1.0074x faster

                                                         Baseline              GCDontDropLocks                                  
Kraken:
   ai-astar                                          831.922+-10.318           818.597+-21.629          might be 1.0163x faster
   audio-beat-detection                              221.653+-5.461            221.273+-1.316         
   audio-dft                                         295.661+-2.959      ?     302.912+-19.833        ? might be 1.0245x slower
   audio-fft                                         155.251+-0.618      ?     155.711+-0.351         ?
   audio-oscillator                                  438.332+-3.393            436.554+-1.882         
   imaging-darkroom                                  334.565+-1.615      ?     335.556+-2.214         ?
   imaging-desaturate                                144.874+-0.283      ?     145.078+-0.613         ?
   imaging-gaussian-blur                             216.790+-0.959            216.398+-0.386         
   json-parse-financial                               92.422+-1.096             92.126+-0.611         
   json-stringify-tinderbox                          117.408+-11.631           111.417+-0.557           might be 1.0538x faster
   stanford-crypto-aes                               118.359+-1.730      ?     118.932+-2.909         ?
   stanford-crypto-ccm                               100.800+-12.170     ?     105.620+-20.493        ? might be 1.0478x slower
   stanford-crypto-pbkdf2                            315.251+-5.126      ?     317.996+-4.245         ?
   stanford-crypto-sha256-iterative                   95.067+-0.568      ?      95.376+-2.879         ?

   <arithmetic> *                                    248.454+-1.219            248.110+-2.814           might be 1.0014x faster
   <geometric>                                       198.810+-1.670      ?     198.998+-3.135         ? might be 1.0009x slower
   <harmonic>                                        167.095+-2.223      ?     167.292+-4.056         ? might be 1.0012x slower

                                                         Baseline              GCDontDropLocks                                  
JSRegress:
   abs-boolean                                        4.9039+-0.7798            4.6655+-0.1388          might be 1.0511x faster
   adapt-to-double-divide                            19.7751+-0.1035           19.7607+-0.1161        
   aliased-arguments-getbyval                         1.4850+-0.0498     ?      1.4988+-0.0874        ?
   allocate-big-object                                3.9099+-0.1903     ?      3.9507+-0.6173        ? might be 1.0104x slower
   arity-mismatch-inlining                            1.3118+-0.1009     ?      1.3835+-0.0940        ? might be 1.0546x slower
   array-access-polymorphic-structure                12.6620+-0.3088     ?     12.6750+-0.2490        ?
   array-nonarray-polymorhpic-access                 68.9453+-0.3822     ?     69.4298+-1.2578        ?
   array-prototype-every                            187.5782+-8.9778          184.9826+-4.2053          might be 1.0140x faster
   array-prototype-forEach                          183.1628+-7.2041          182.8010+-1.7654        
   array-prototype-map                              224.4810+-6.0747          224.0670+-7.1083        
   array-prototype-some                             189.9885+-2.2155     ?    191.6420+-6.1883        ?
   array-splice-contiguous                           95.0856+-4.1395           95.0295+-4.8040        
   array-with-double-add                              7.4538+-0.1129            7.4410+-0.2286        
   array-with-double-increment                        5.6599+-0.2689            5.5112+-0.2261          might be 1.0270x faster
   array-with-double-mul-add                         11.8201+-0.2235           11.6105+-0.1533          might be 1.0181x faster
   array-with-double-sum                              5.4011+-0.1249     ?      5.4792+-0.1940        ? might be 1.0145x slower
   array-with-int32-add-sub                          13.1190+-0.1629           13.0753+-0.0849        
   array-with-int32-or-double-sum                     5.3206+-0.1428     ?      5.3509+-0.0814        ?
   ArrayBuffer-DataView-alloc-large-long-lived   
                                                     67.4449+-3.1558           65.0034+-1.2483          might be 1.0376x faster
   ArrayBuffer-DataView-alloc-long-lived             28.3835+-2.6615           26.8492+-0.4847          might be 1.0571x faster
   ArrayBuffer-Int32Array-byteOffset                  5.1708+-0.1793     ?      5.3670+-0.2430        ? might be 1.0379x slower
   ArrayBuffer-Int8Array-alloc-large-long-lived   
                                                     69.3553+-1.3089           68.3317+-1.2618          might be 1.0150x faster
   ArrayBuffer-Int8Array-alloc-long-lived-buffer   
                                                     49.0463+-3.5148           45.8782+-5.1571          might be 1.0691x faster
   ArrayBuffer-Int8Array-alloc-long-lived            25.9260+-1.2349           25.6655+-0.3735          might be 1.0102x faster
   ArrayBuffer-Int8Array-alloc                       21.1623+-0.4510     ?     21.2051+-0.5032        ?
   asmjs_bool_bug                                    12.7102+-0.2582     ?     12.9255+-0.3227        ? might be 1.0169x slower
   assign-custom-setter-polymorphic                   6.0051+-0.2325     ?      6.1526+-0.2088        ? might be 1.0246x slower
   assign-custom-setter                               8.3787+-0.3240            8.1545+-0.0807          might be 1.0275x faster
   basic-set                                         17.6423+-1.1215           17.5315+-0.9472        
   big-int-mul                                        7.7305+-0.1298            7.7203+-0.1079        
   boolean-test                                       4.9650+-0.0958            4.9219+-0.1432        
   branch-fold                                        5.2761+-0.1207            5.2139+-0.1087          might be 1.0119x faster
   by-val-generic                                    15.6538+-0.4113     ?     15.8600+-0.7473        ? might be 1.0132x slower
   call-spread-apply                                 25.7766+-0.3250     ?     26.1796+-1.0617        ? might be 1.0156x slower
   call-spread-call                                  11.3736+-0.2834     ?     11.4882+-0.2004        ? might be 1.0101x slower
   captured-assignments                               0.8073+-0.1250     ?      0.8228+-0.1541        ? might be 1.0191x slower
   cast-int-to-double                                 8.6315+-0.1372     ?      8.6738+-0.1119        ?
   cell-argument                                     12.0446+-0.2477     ?     12.2531+-0.4238        ? might be 1.0173x slower
   cfg-simplify                                       3.9150+-0.0722     ?      3.9724+-0.0507        ? might be 1.0147x slower
   chain-getter-access                               16.4993+-0.3234           16.4705+-0.2705        
   cmpeq-obj-to-obj-other                            16.6860+-0.4798     ?     16.7102+-0.4701        ?
   constant-test                                      8.0961+-0.0803            8.0090+-0.1410          might be 1.0109x faster
   DataView-custom-properties                        79.4473+-4.3748           76.1459+-3.1501          might be 1.0434x faster
   delay-tear-off-arguments-strictmode               45.4232+-0.9265     ?     45.5005+-0.9970        ?
   destructuring-arguments                            9.1252+-0.1275            9.1111+-0.2100        
   destructuring-swap                                 8.3210+-0.1561            8.2637+-0.1086        
   direct-arguments-getbyval                          1.5987+-0.3076     ?      1.6596+-0.3791        ? might be 1.0381x slower
   div-boolean-double                                 5.8640+-0.0654     ?      5.9987+-0.1347        ? might be 1.0230x slower
   div-boolean                                       10.2576+-0.0393           10.2510+-0.3121        
   double-get-by-val-out-of-bounds                    8.1508+-0.2113     ?      8.2041+-0.2906        ?
   double-pollution-getbyval                         10.0820+-0.0961     ?     10.1068+-0.1696        ?
   double-pollution-putbyoffset                       8.0867+-0.1528            8.0607+-0.0867        
   double-to-int32-typed-array-no-inline              3.7795+-0.2651            3.7372+-0.0584          might be 1.0113x faster
   double-to-int32-typed-array                        3.2941+-0.1358            3.2575+-0.0923          might be 1.0112x faster
   double-to-uint32-typed-array-no-inline             3.8181+-0.1766     ?      3.8832+-0.2454        ? might be 1.0170x slower
   double-to-uint32-typed-array                       3.3947+-0.1728            3.2814+-0.1806          might be 1.0345x faster
   elidable-new-object-dag                           62.6877+-2.1583           62.4510+-2.5799        
   elidable-new-object-roflcopter                   242.1007+-5.5626          237.9272+-2.5464          might be 1.0175x faster
   elidable-new-object-then-call                     60.3154+-2.0250           58.7932+-0.9074          might be 1.0259x faster
   elidable-new-object-tree                          70.2939+-1.2846     ?     70.3883+-1.1415        ?
   empty-string-plus-int                             10.4761+-0.4592           10.2294+-0.2269          might be 1.0241x faster
   emscripten-cube2hash                              57.2521+-1.1727     ?     58.2951+-1.4164        ? might be 1.0182x slower
   external-arguments-getbyval                        2.5018+-0.1807            2.4797+-0.3049        
   external-arguments-putbyval                        3.8517+-0.1520            3.7413+-0.2808          might be 1.0295x faster
   fixed-typed-array-storage-var-index                2.0370+-0.0943            1.9715+-0.1770          might be 1.0332x faster
   fixed-typed-array-storage                          1.3853+-0.1255     ?      1.4150+-0.1272        ? might be 1.0215x slower
   Float32Array-matrix-mult                           8.4427+-0.6791            8.0106+-0.2693          might be 1.0539x faster
   Float32Array-to-Float64Array-set                 117.7137+-2.0385     ?    119.9370+-7.1261        ? might be 1.0189x slower
   Float64Array-alloc-long-lived                    104.3427+-0.7615          103.9674+-0.5007        
   Float64Array-to-Int16Array-set                   147.8989+-5.7667          145.5093+-2.5894          might be 1.0164x faster
   fold-double-to-int                                24.5105+-0.5455           24.3843+-0.3457        
   fold-get-by-id-to-multi-get-by-offset-rare-int   
                                                     14.3041+-0.5144           14.0502+-0.2147          might be 1.0181x faster
   fold-get-by-id-to-multi-get-by-offset             12.2864+-0.3936     ?     12.4200+-0.5803        ? might be 1.0109x slower
   fold-multi-get-by-offset-to-get-by-offset   
                                                     10.5012+-1.4849     ?     11.6681+-0.8244        ? might be 1.1111x slower
   fold-multi-get-by-offset-to-poly-get-by-offset   
                                                     11.3296+-1.2084     ?     11.4252+-0.9003        ?
   fold-multi-put-by-offset-to-poly-put-by-offset   
                                                     10.5042+-1.1953            9.9715+-1.1096          might be 1.0534x faster
   fold-multi-put-by-offset-to-put-by-offset   
                                                      8.6668+-1.5594     ?      8.7687+-1.0436        ? might be 1.0118x slower
   fold-multi-put-by-offset-to-replace-or-transition-put-by-offset   
                                                     15.4473+-2.0953     ?     15.7321+-1.4008        ? might be 1.0184x slower
   fold-put-by-id-to-multi-put-by-offset             11.4315+-0.8821     ?     11.7145+-0.8120        ? might be 1.0248x slower
   fold-put-structure                                 8.6942+-1.1696     ?      8.8929+-1.0738        ? might be 1.0229x slower
   for-of-iterate-array-entries                      11.0456+-0.4550     ?     11.1745+-0.4394        ? might be 1.0117x slower
   for-of-iterate-array-keys                          5.1106+-0.1261     ?      5.3860+-0.3469        ? might be 1.0539x slower
   for-of-iterate-array-values                        4.3795+-0.3280     ?      4.7305+-0.2394        ? might be 1.0801x slower
   fround                                            26.1556+-1.0184           25.7432+-0.0276          might be 1.0160x faster
   ftl-library-inlining-dataview                    159.5065+-20.3904    ?    166.5156+-27.6958       ? might be 1.0439x slower
   ftl-library-inlining                             134.2111+-3.2031          133.3567+-0.3098        
   function-dot-apply                                 2.8790+-0.1053            2.7940+-0.2984          might be 1.0304x faster
   function-test                                      5.6203+-0.2980            5.5601+-0.1176          might be 1.0108x faster
   function-with-eval                               226.4252+-1.1450     ?    226.6873+-4.2426        ?
   gcse-poly-get-less-obvious                        36.7097+-0.9344     ?     37.1866+-0.7438        ? might be 1.0130x slower
   gcse-poly-get                                     36.6384+-0.2324     ?     36.9536+-0.2619        ?
   gcse                                               9.1887+-0.1535            9.1263+-0.1787        
   get-by-id-bimorphic-check-structure-elimination-simple   
                                                      4.1465+-0.1181            4.0878+-0.0447          might be 1.0144x faster
   get-by-id-bimorphic-check-structure-elimination   
                                                     12.1324+-0.1044           12.0778+-0.1025        
   get-by-id-chain-from-try-block                    19.7584+-0.1127           19.6074+-0.0805        
   get-by-id-check-structure-elimination             10.7896+-0.1714           10.7125+-0.1162        
   get-by-id-proto-or-self                           33.2939+-4.5872           30.6615+-0.5304          might be 1.0859x faster
   get-by-id-quadmorphic-check-structure-elimination-simple   
                                                      5.1968+-0.1885     ?      5.3729+-0.3389        ? might be 1.0339x slower
   get-by-id-self-or-proto                           32.3140+-3.8927     ?     32.4664+-4.1033        ?
   get-by-val-out-of-bounds                           7.7048+-0.0937     ?      7.8212+-0.1789        ? might be 1.0151x slower
   get_callee_monomorphic                             6.4418+-0.5322     ?      6.5833+-0.9700        ? might be 1.0220x slower
   get_callee_polymorphic                             5.3914+-0.1723     ?      5.5063+-0.0768        ? might be 1.0213x slower
   getter-no-activation                               6.3140+-0.1034     ?      6.3192+-0.1278        ?
   getter-richards                                  180.3203+-3.7006          176.7853+-7.2642          might be 1.0200x faster
   getter                                             9.2660+-0.4927            9.2566+-0.4562        
   global-var-const-infer-fire-from-opt               1.6943+-0.0592     ?      1.8022+-0.0847        ? might be 1.0637x slower
   global-var-const-infer                             1.7415+-0.1462            1.6291+-0.1500          might be 1.0690x faster
   HashMap-put-get-iterate-keys                      51.6534+-1.1998     ?     51.7634+-1.0618        ?
   HashMap-put-get-iterate                           50.3761+-0.6012           50.0996+-0.8934        
   HashMap-string-put-get-iterate                    48.1560+-0.6403           47.3760+-0.2342          might be 1.0165x faster
   hoist-make-rope                                   17.6647+-2.6264     ?     19.1873+-3.1359        ? might be 1.0862x slower
   hoist-poly-check-structure-effectful-loop   
                                                      9.5908+-0.1093     ?      9.6189+-0.0365        ?
   hoist-poly-check-structure                         6.3116+-0.1337     ?      6.3143+-0.1068        ?
   imul-double-only                                  13.8478+-2.1592           12.1512+-0.3126          might be 1.1396x faster
   imul-int-only                                     14.3560+-0.7743     ?     14.5774+-0.0720        ? might be 1.0154x slower
   imul-mixed                                        12.2758+-0.3281     ?     12.4580+-0.1690        ? might be 1.0148x slower
   in-four-cases                                     35.3623+-0.2299     ?     35.5377+-0.3855        ?
   in-one-case-false                                 18.7362+-0.1230     ?     18.9167+-0.0904        ?
   in-one-case-true                                  18.7361+-0.2161     ?     18.7432+-0.1037        ?
   in-two-cases                                      19.2914+-0.2106           19.2202+-0.0497        
   indexed-properties-in-objects                      4.7818+-0.0994            4.7349+-0.1446        
   infer-closure-const-then-mov-no-inline             6.0425+-0.0580            5.9664+-0.0912          might be 1.0128x faster
   infer-closure-const-then-mov                      26.1909+-0.2291           26.0888+-0.2013        
   infer-closure-const-then-put-to-scope-no-inline   
                                                     19.9008+-0.1173           19.8727+-0.1765        
   infer-closure-const-then-put-to-scope             33.9648+-0.9965     ?     34.1788+-0.7592        ?
   infer-closure-const-then-reenter-no-inline   
                                                     93.0925+-0.2930           92.6370+-0.8677        
   infer-closure-const-then-reenter                  31.8092+-0.5085     ?     31.8775+-0.2082        ?
   infer-constant-global-property                    38.8815+-0.0816     ?     38.9991+-0.1381        ?
   infer-constant-property                            3.7441+-0.1425            3.6388+-0.2008          might be 1.0289x faster
   infer-one-time-closure-ten-vars                   17.6861+-0.3930           17.6125+-0.1956        
   infer-one-time-closure-two-vars                   16.9491+-0.1937     ?     16.9615+-0.5703        ?
   infer-one-time-closure                            16.8242+-0.4588           16.7938+-0.5082        
   infer-one-time-deep-closure                       29.8387+-0.2992           29.5775+-0.5055        
   inline-arguments-access                            2.8828+-0.0841            2.8301+-0.1709          might be 1.0186x faster
   inline-arguments-aliased-access                    3.0391+-0.1706     ?      3.2081+-0.0639        ? might be 1.0556x slower
   inline-arguments-local-escape                     26.2448+-0.2466     ?     26.6507+-1.6650        ? might be 1.0155x slower
   inline-get-scoped-var                              5.9337+-0.2037            5.9230+-0.1995        
   inlined-put-by-id-transition                      18.6192+-0.2993     ?     18.6315+-0.2925        ?
   int-or-other-abs-then-get-by-val                   9.5482+-0.1762            9.4762+-0.1543        
   int-or-other-abs-zero-then-get-by-val             34.4332+-0.5905     ?     34.4520+-0.8143        ?
   int-or-other-add-then-get-by-val                   8.4808+-0.1908            8.4376+-0.2232        
   int-or-other-add                                   9.0556+-0.2322     ?      9.1369+-0.3095        ?
   int-or-other-div-then-get-by-val                   6.5219+-0.1306            6.5076+-0.1235        
   int-or-other-max-then-get-by-val                   8.0834+-0.2801     ?      8.2429+-0.3038        ? might be 1.0197x slower
   int-or-other-min-then-get-by-val                   7.2509+-0.1862            7.2227+-0.2425        
   int-or-other-mod-then-get-by-val                   6.1520+-0.0339            6.1027+-0.1393        
   int-or-other-mul-then-get-by-val                   6.5627+-0.0828            6.5493+-0.1748        
   int-or-other-neg-then-get-by-val                   8.1566+-0.2424            8.1215+-0.2293        
   int-or-other-neg-zero-then-get-by-val             34.5405+-0.2778           34.1718+-0.4726          might be 1.0108x faster
   int-or-other-sub-then-get-by-val                   8.8137+-0.0916            8.7767+-0.2080        
   int-or-other-sub                                   6.8842+-0.1051            6.7810+-0.7515          might be 1.0152x faster
   int-overflow-local                                 7.9830+-0.1900            7.9332+-0.1485        
   Int16Array-alloc-long-lived                       77.7075+-0.9676           76.7988+-0.7093          might be 1.0118x faster
   Int16Array-bubble-sort-with-byteLength            51.2611+-0.4691           51.1688+-0.2292        
   Int16Array-bubble-sort                            49.8958+-0.3509           49.7271+-0.3523        
   Int16Array-load-int-mul                            2.4927+-0.3321            2.3790+-0.1765          might be 1.0478x faster
   Int16Array-to-Int32Array-set                     117.2755+-5.7481          116.0438+-8.2719          might be 1.0106x faster
   Int32Array-alloc-large                            37.8079+-0.7289           37.5195+-0.3693        
   Int32Array-alloc-long-lived                       84.8037+-0.6696           84.3643+-0.5691        
   Int32Array-alloc                                   4.9630+-0.1617     ?      5.0410+-0.1938        ? might be 1.0157x slower
   Int32Array-Int8Array-view-alloc                   13.2523+-1.1704           13.0984+-0.5665          might be 1.0117x faster
   int52-spill                                       12.6750+-0.3402     ?     12.7317+-0.2315        ?
   Int8Array-alloc-long-lived                        71.4784+-0.8167           71.2983+-0.1291        
   Int8Array-load-with-byteLength                     5.1890+-0.1435            5.1741+-0.1626        
   Int8Array-load                                     5.2203+-0.1241            5.1931+-0.1744        
   integer-divide                                    19.4474+-0.1624           19.2855+-0.0699        
   integer-modulo                                     3.7591+-0.3421     ?      3.7742+-0.1617        ?
   large-int-captured                                12.5248+-0.3290     ?     12.7825+-0.5772        ? might be 1.0206x slower
   large-int-neg                                     30.6542+-0.4530           30.5390+-0.3885        
   large-int                                         27.8512+-0.2662           27.7445+-0.1375        
   logical-not                                        8.0995+-0.1499     ?      8.3277+-0.5641        ? might be 1.0282x slower
   lots-of-fields                                    19.5097+-0.1337           19.4719+-0.3008        
   make-indexed-storage                               5.3522+-0.2227            4.8837+-0.7897          might be 1.0959x faster
   make-rope-cse                                      6.3777+-0.2408     ?      6.5562+-0.4396        ? might be 1.0280x slower
   marsaglia-larger-ints                             69.2272+-1.1604           68.7192+-1.3240        
   marsaglia-osr-entry                               34.6951+-0.5846           34.5122+-0.3559        
   max-boolean                                        3.9263+-0.0762            3.9242+-0.1351        
   method-on-number                                  34.1494+-0.7148     ?     35.0854+-3.5781        ? might be 1.0274x slower
   min-boolean                                        3.8738+-0.1329            3.8368+-0.1615        
   minus-boolean-double                               4.3873+-0.1232            4.3200+-0.0727          might be 1.0156x faster
   minus-boolean                                      3.7852+-0.0720            3.7033+-0.1755          might be 1.0221x faster
   misc-strict-eq                                    71.0651+-1.1205     ?     76.1788+-9.0995        ? might be 1.0720x slower
   mod-boolean-double                                14.3724+-0.1671     ?     14.5200+-0.8415        ? might be 1.0103x slower
   mod-boolean                                        9.2263+-0.1184            9.2256+-0.1656        
   mul-boolean-double                                 4.9919+-0.1161     ?      5.0269+-0.1229        ?
   mul-boolean                                        4.1248+-0.0623            4.0256+-0.1719          might be 1.0246x faster
   neg-boolean                                        4.5883+-0.0942     ?      4.6048+-0.0437        ?
   negative-zero-divide                               0.6188+-0.1386     ?      0.6452+-0.0898        ? might be 1.0426x slower
   negative-zero-modulo                               0.6545+-0.0473            0.6349+-0.1155          might be 1.0308x faster
   negative-zero-negate                               0.5855+-0.0879            0.5372+-0.0249          might be 1.0899x faster
   nested-function-parsing                           41.0352+-0.2690           40.8641+-0.2172        
   new-array-buffer-dead                              4.1129+-0.2076     ?      4.1379+-0.3367        ?
   new-array-buffer-push                             10.7457+-0.2452           10.5203+-0.1413          might be 1.0214x faster
   new-array-dead                                    17.6908+-0.9714           17.5629+-0.9695        
   new-array-push                                     8.3011+-0.1249     ?      8.3193+-0.2714        ?
   number-test                                        4.9020+-0.4426            4.8317+-0.2495          might be 1.0145x faster
   object-closure-call                               10.5706+-0.1870     ?     10.7538+-0.0905        ? might be 1.0173x slower
   object-test                                        5.3610+-0.2113     ?      5.3730+-0.1623        ?
   obvious-sink-pathology-taken                     217.3514+-1.9245     ^    214.0780+-0.3054        ^ definitely 1.0153x faster
   obvious-sink-pathology                           201.2391+-2.6274          200.7522+-3.5779        
   obviously-elidable-new-object                     55.7850+-0.9496           55.0394+-0.7010          might be 1.0135x faster
   plus-boolean-arith                                 4.0974+-0.1436            4.0213+-0.0837          might be 1.0189x faster
   plus-boolean-double                                4.4755+-0.0946            4.4678+-0.0669        
   plus-boolean                                       3.5936+-0.1286     ?      3.6833+-0.2692        ? might be 1.0250x slower
   poly-chain-access-different-prototypes-simple   
                                                      5.1332+-0.0970            5.1332+-0.0854        
   poly-chain-access-different-prototypes             3.7897+-0.7005            3.5861+-0.2065          might be 1.0568x faster
   poly-chain-access-simpler                          5.1768+-0.1205     ?      5.1812+-0.1303        ?
   poly-chain-access                                  3.5493+-0.1897            3.5275+-0.1414        
   poly-stricteq                                    101.1295+-2.5868          100.9315+-1.1071        
   polymorphic-array-call                             3.3520+-0.2910            3.2274+-0.1553          might be 1.0386x faster
   polymorphic-get-by-id                              5.2880+-0.1104            5.2699+-0.1166        
   polymorphic-put-by-id                             46.4290+-2.5495     ?     46.7411+-2.5766        ?
   polymorphic-structure                             32.9877+-0.1825           32.7559+-0.3972        
   polyvariant-monomorphic-get-by-id                 17.1022+-1.0412           16.8517+-0.2065          might be 1.0149x faster
   proto-getter-access                               16.7443+-0.4297           16.5272+-0.3409          might be 1.0131x faster
   put-by-id-replace-and-transition                  13.8135+-0.1142     ^     13.4194+-0.0740        ^ definitely 1.0294x faster
   put-by-id-slightly-polymorphic                     4.2306+-0.2011            4.0024+-0.1129          might be 1.0570x faster
   put-by-id                                         20.6379+-0.6266           20.5039+-0.3636        
   put-by-val-direct                                  1.0159+-0.1372            0.9672+-0.0237          might be 1.0504x faster
   put-by-val-large-index-blank-indexing-type   
                                                     10.8410+-0.4722     ?     11.1888+-1.0078        ? might be 1.0321x slower
   put-by-val-machine-int                             4.3951+-0.2577            4.2426+-0.2174          might be 1.0359x faster
   rare-osr-exit-on-local                            25.5859+-0.2137           25.5680+-0.0928        
   register-pressure-from-osr                        38.1308+-0.3462     ?     38.8505+-1.8452        ? might be 1.0189x slower
   setter                                             7.2527+-0.0726            7.2230+-0.1075        
   simple-activation-demo                            45.2758+-0.9283           44.9360+-0.4936        
   simple-getter-access                              22.1892+-0.5301           22.1487+-0.3947        
   simple-poly-call-nested                           13.9409+-0.7088           13.7840+-0.1337          might be 1.0114x faster
   simple-poly-call                                   2.0372+-0.1697     ?      2.0515+-0.0422        ?
   sin-boolean                                       29.6371+-0.4665           29.5812+-0.5918        
   sinkable-new-object-dag                          117.4052+-0.9555          116.5355+-0.4832        
   sinkable-new-object-taken                         93.4968+-5.9617           88.9117+-0.6971          might be 1.0516x faster
   sinkable-new-object                               61.2180+-0.8399     ?     61.3400+-1.4976        ?
   slow-array-profile-convergence                     5.1018+-0.2715     ?      5.3026+-0.1663        ? might be 1.0394x slower
   slow-convergence                                   5.7303+-0.3998            5.6806+-0.4248        
   sparse-conditional                                 1.8288+-0.1456     ?      1.8464+-0.2429        ?
   splice-to-remove                                  41.6455+-0.8918     ^     35.4322+-2.6289        ^ definitely 1.1754x faster
   string-char-code-at                               30.4093+-0.3345           30.2682+-0.3253        
   string-concat-object                               3.3947+-0.2665            3.3003+-0.2673          might be 1.0286x faster
   string-concat-pair-object                          3.4307+-0.2015     ?      3.4792+-0.4550        ? might be 1.0141x slower
   string-concat-pair-simple                         19.2665+-0.5413           18.7865+-0.5862          might be 1.0256x faster
   string-concat-simple                              19.9454+-0.3921           19.6539+-0.6129          might be 1.0148x faster
   string-cons-repeat                                12.4667+-0.4590           12.1314+-0.3106          might be 1.0276x faster
   string-cons-tower                                 12.0494+-0.2723           11.5101+-0.3408          might be 1.0469x faster
   string-equality                                   34.7372+-0.6414     ?     35.0696+-1.7369        ?
   string-get-by-val-big-char                        13.5543+-0.1765           13.3838+-0.1537          might be 1.0127x faster
   string-get-by-val-out-of-bounds-insane             8.4212+-0.1948     ?      8.8743+-1.9815        ? might be 1.0538x slower
   string-get-by-val-out-of-bounds                   10.1138+-0.6395            9.8272+-0.1071          might be 1.0292x faster
   string-get-by-val                                  6.8783+-0.0854            6.7597+-0.1664          might be 1.0175x faster
   string-hash                                        3.5449+-0.0487     ?      3.6699+-0.1223        ? might be 1.0352x slower
   string-long-ident-equality                        27.2795+-1.1739           27.0955+-0.6206        
   string-repeat-arith                               62.1400+-1.3519           61.9624+-0.5083        
   string-sub                                       120.3463+-0.8595     ?    120.8298+-0.3453        ?
   string-test                                        4.8938+-0.2348            4.7329+-0.1307          might be 1.0340x faster
   string-var-equality                               62.1268+-0.3333           62.0968+-0.2072        
   structure-hoist-over-transitions                   4.1060+-0.2878     ?      4.1290+-0.2895        ?
   substring-concat-weird                            74.5724+-0.8467           73.9078+-0.6376        
   substring-concat                                  78.8601+-0.6116           78.1986+-0.4697        
   substring                                         90.0316+-0.3427           89.7267+-0.3882        
   switch-char-constant                               3.8243+-0.2032            3.8170+-0.1772        
   switch-char                                       12.2530+-1.0250           12.0240+-0.1224          might be 1.0190x faster
   switch-constant                                   19.1667+-1.1769           18.3155+-0.1905          might be 1.0465x faster
   switch-string-basic-big-var                       27.8137+-1.4937           27.0532+-0.2000          might be 1.0281x faster
   switch-string-basic-big                           23.8311+-0.3789           23.6327+-0.3626        
   switch-string-basic-var                           32.9052+-1.6893           32.4160+-1.4665          might be 1.0151x faster
   switch-string-basic                               26.6434+-2.2716     ?     27.9477+-1.5121        ? might be 1.0490x slower
   switch-string-big-length-tower-var                31.2447+-0.1660     ?     31.3659+-0.3409        ?
   switch-string-length-tower-var                    26.1968+-0.2505           26.1537+-0.7635        
   switch-string-length-tower                        20.9168+-0.2720     ?     21.0845+-0.3291        ?
   switch-string-short                               20.3360+-0.4472           20.2965+-0.1721        
   switch                                            27.1509+-0.8038           23.6587+-6.6594          might be 1.1476x faster
   tear-off-arguments-simple                          3.0574+-0.2098            3.0242+-0.2302          might be 1.0110x faster
   tear-off-arguments                                 4.6716+-0.1961            4.5760+-0.1241          might be 1.0209x faster
   temporal-structure                                20.7015+-0.4431           20.6696+-0.1768        
   to-int32-boolean                                  24.9670+-0.2490     ?     25.0707+-0.1293        ?
   undefined-property-access                        582.8043+-1.7300     ?    584.1109+-5.1334        ?
   undefined-test                                     4.9273+-0.0649     ?      5.0392+-0.0795        ? might be 1.0227x slower
   unprofiled-licm                                   34.8692+-0.5498           34.5882+-0.1922        
   weird-inlining-const-prop                          3.3654+-0.2688     ?      3.4314+-0.1782        ? might be 1.0196x slower

   <arithmetic>                                      30.7965+-0.0675           30.6547+-0.1106          might be 1.0046x faster
   <geometric> *                                     13.7433+-0.0428           13.6871+-0.0692          might be 1.0041x faster
   <harmonic>                                         6.8824+-0.1087            6.8480+-0.0891          might be 1.0050x faster

                                                         Baseline              GCDontDropLocks                                  
AsmBench:
   bigfib.cpp                                       892.8593+-11.0205         884.5098+-12.9579       
   cray.c                                           937.8826+-9.8051          934.5866+-7.5146        
   dry.c                                           1054.9327+-2.1557         1038.5106+-50.3661         might be 1.0158x faster
   FloatMM.c                                       1302.4790+-2.9021     ?   1304.0134+-5.5945        ?
   gcc-loops.cpp                                   8249.8732+-8.1224         8245.3165+-46.8821       
   n-body.c                                        2459.3489+-1.5754     ?   2463.0585+-4.0248        ?
   Quicksort.c                                      711.7990+-12.7685    ?    715.6802+-2.4469        ?
   stepanov_container.cpp                          6171.9437+-126.7963   ?   6200.2064+-103.5813      ?
   Towers.c                                         604.5075+-1.6238     ?    605.1746+-1.3776        ?

   <arithmetic>                                    2487.2918+-15.4417    ?   2487.8952+-12.5238       ? might be 1.0002x slower
   <geometric> *                                   1581.7820+-6.6192         1579.0328+-7.2450          might be 1.0017x faster
   <harmonic>                                      1176.2988+-4.4010         1173.5011+-5.4085          might be 1.0024x faster

                                                         Baseline              GCDontDropLocks                                  
CompressionBench:
   huffman                                         1053.0552+-7.4523         1052.2343+-7.6608        
   arithmetic-simple                                767.8580+-6.4668     ?    771.0495+-4.0561        ?
   arithmetic-precise                               594.5696+-4.5895     ?    594.6685+-4.6868        ?
   arithmetic-complex-precise                       597.4836+-8.7574          596.3629+-9.3928        
   arithmetic-precise-order-0                       888.8469+-12.1076    ?    892.4737+-3.0690        ?
   arithmetic-precise-order-1                       649.8916+-6.8364     ?    655.4582+-23.2516       ?
   arithmetic-precise-order-2                       737.1080+-39.4436         701.8575+-11.0504         might be 1.0502x faster
   arithmetic-simple-order-1                        799.6346+-29.9775         793.8840+-17.2383       
   arithmetic-simple-order-2                        875.8927+-5.6488          872.1931+-15.2694       
   lz-string                                        633.1583+-30.5944         631.7170+-28.3004       

   <arithmetic>                                     759.7499+-7.8170          756.1899+-2.6853          might be 1.0047x faster
   <geometric> *                                    747.1227+-8.4012          743.5275+-3.0568          might be 1.0048x faster
   <harmonic>                                       735.1799+-8.8974          731.6467+-3.4965          might be 1.0048x faster

                                                         Baseline              GCDontDropLocks                                  
All benchmarks:
   <arithmetic>                                     188.5052+-0.1356          188.4297+-0.1818          might be 1.0004x faster
   <geometric>                                       23.4283+-0.0664           23.3506+-0.0895          might be 1.0033x faster
   <harmonic>                                         5.9626+-0.0663            5.9286+-0.0509          might be 1.0057x faster

                                                         Baseline              GCDontDropLocks                                  
Geomean of preferred means:
   <scaled-result>                                  120.8835+-0.4227          120.6260+-0.2997          might be 1.0021x faster

[Mark Lam]

Comment on attachment 246072 [details]
Patch

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

I'm still not sure about the effects on peak memory usage for ObjC apps that uses long running JS code which also allocates a lot of JS wrapped ObjC objects.  However, it is re-assuring that release of JS objects isn't gated by this change.  I agree that we can go with this solution until there is evidence to the contrary that the peak memory usage is of consequence.

I have added a few more questions and comments.  See below.

>>> Source/JavaScriptCore/ChangeLog:7
>>> +
>> 
>> Your ChangeLog comment describes what was done but not why.  I think your description of the issue in bugzilla did a good job of explaining what the issue is.  Would you mind adding it here before you start explaining how it is fixed?
> 
> I added this paragraph to the top of the ChangeLog entry:
>     The root issue is that one thread, while allocating JS objects may need to garbage collect.
>     During that garbage collection, if there are delayed Objective C objects that need to be
>     released, we drop the locks to release them.  While the locks are release by that thread,
>     another thread can enter the VM and might have exactly the same source and therefore hit
>     this issue.  This fixes the problem by not dropping the locks during garbage collection.

I feel that the detail about the code cache having a partially initialized entry is important to understanding this issue.  How about ...

    The root issue is that one thread, while parsing source code into an UnlinkedCodeBlock, may need to garbage collect. During that garbage collection, if there are delayed Objective C objects that need to be released, we drop the locks to release them. While the locks are release by that thread, another thread can enter the VM and might parse exactly the same source and therefore hit this issue where the CodeCache contains an incomplete entry for that source from the first thread. This fixes the problem by not dropping the locks during garbage collection.

>>> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:1758
>>> +</Project>
>> 
>> Is this necessary and ok to do?
> 
> Given that it is XML, I think it is fine.  The Win EWS bot built with the patch just fine.

Would you mind reverting this line, or at least deleting the "\No newline at end of file"?  This change is just not relevant to this patch, and having the "\No newline at end of file" before the end of the file just feels icky.

>>> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:4390
>>> +</Project>
>> 
>> Ditto.
> 
> Fixed.

Ditto if your fixed does not mean reverting this change, or deleting the "\No newline at end of file".

> Source/JavaScriptCore/heap/Heap.cpp:376
> +    if (!m_delayedReleaseRecursionCount++) {
> +        while (!m_delayedReleaseObjects.isEmpty()) {
> +            RetainPtr<CFTypeRef> objectToRelease = m_delayedReleaseObjects.takeLast();
> +            objectToRelease.clear();
> +        }
> +    }

In the original ~DelayedReleaseScope(), we do the release by simply calling m_delayedReleaseObjects.clear().  Why do we need to iterate it here and manually clear each entry?

Another question: why do we need this m_delayedReleaseRecursionCount?  Given that releaseDelayedReleasedObjects() is only called just before the outermost JSLock is unlocked, and on heap destruction, there should be no chance of m_delayedReleaseRecursionCount ever being more than 0.  Am I missing something?

> Source/JavaScriptCore/heap/MarkedAllocator.cpp:132
> -    // Due to the DelayedReleaseScope in tryAllocateHelper, some other thread might have
> +    // Due to the DelayedReleaseList in tryAllocateHelper, some other thread might have
>      // created a new block after we thought we didn't find any free cells. 
>      while (!result && m_currentBlock) {
>          // A new block was added by another thread so try popping the free list.

I think this while loop is now obsolete, and this comment is invalid.  Since we no longer have a DelayedReleaseScope in tryAllocateHelper(), there's now no opportunity for another thread to run and create the new block that we're checking for here.  Hence, this loop and comment should be removed.

[Michael Saboff]

(In reply to comment #5)
> Comment on attachment 246072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246072&action=review
> 
> I'm still not sure about the effects on peak memory usage for ObjC apps that
> uses long running JS code which also allocates a lot of JS wrapped ObjC
> objects.  However, it is re-assuring that release of JS objects isn't gated
> by this change.  I agree that we can go with this solution until there is
> evidence to the contrary that the peak memory usage is of consequence.
> 
> I have added a few more questions and comments.  See below.
> 
> >>> Source/JavaScriptCore/ChangeLog:7
> >>> +
> >> 
> >> Your ChangeLog comment describes what was done but not why.  I think your description of the issue in bugzilla did a good job of explaining what the issue is.  Would you mind adding it here before you start explaining how it is fixed?
> > 
> > I added this paragraph to the top of the ChangeLog entry:
> >     The root issue is that one thread, while allocating JS objects may need to garbage collect.
> >     During that garbage collection, if there are delayed Objective C objects that need to be
> >     released, we drop the locks to release them.  While the locks are release by that thread,
> >     another thread can enter the VM and might have exactly the same source and therefore hit
> >     this issue.  This fixes the problem by not dropping the locks during garbage collection.
> 
> I feel that the detail about the code cache having a partially initialized
> entry is important to understanding this issue.  How about ...
> 
>     The root issue is that one thread, while parsing source code into an
> UnlinkedCodeBlock, may need to garbage collect. During that garbage
> collection, if there are delayed Objective C objects that need to be
> released, we drop the locks to release them. While the locks are release by
> that thread, another thread can enter the VM and might parse exactly the
> same source and therefore hit this issue where the CodeCache contains an
> incomplete entry for that source from the first thread. This fixes the
> problem by not dropping the locks during garbage collection.

During GC, we used to drop all lock even if there aren't any delayed Objective C objects that need to be released.

How about:

    The issue for this bug is that one thread, takes a cache miss in CodeCache::getGlobalCodeBlock,
    but in the process creates a cache entry with a nullptr UnlinkedCodeBlockType* which it will
    fill in later in the function.  During the body of that function, it allocates objects  that
    may garbage collect.  During that garbage collection, we drop the all locks.  While the 
    locks are released by the first thread, another thread can enter the VM and might have exactly
    the same source and enter CodeCache::getGlobalCodeBlock() itself.  When it looks up the code block,
    it sees it as a cache it and uses the nullptr UnlinkedCodeBlockType* and crashes.  This fixes
    the problem by not dropping the locks during garbage collection.  There are other likely scenarios
    where we have a date structure like this code cache in an unsafe state for arbitrary reentrance.

> >>> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:1758
> >>> +</Project>
> >> 
> >> Is this necessary and ok to do?
> > 
> > Given that it is XML, I think it is fine.  The Win EWS bot built with the patch just fine.
> 
> Would you mind reverting this line, or at least deleting the "\No newline at
> end of file"?  This change is just not relevant to this patch, and having
> the "\No newline at end of file" before the end of the file just feels icky.

Fine.

> >>> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:4390
> >>> +</Project>
> >> 
> >> Ditto.
> > 
> > Fixed.
> 
> Ditto if your fixed does not mean reverting this change, or deleting the
> "\No newline at end of file".

I'll revert.

> > Source/JavaScriptCore/heap/Heap.cpp:376
> > +    if (!m_delayedReleaseRecursionCount++) {
> > +        while (!m_delayedReleaseObjects.isEmpty()) {
> > +            RetainPtr<CFTypeRef> objectToRelease = m_delayedReleaseObjects.takeLast();
> > +            objectToRelease.clear();
> > +        }
> > +    }
> 
> In the original ~DelayedReleaseScope(), we do the release by simply calling
> m_delayedReleaseObjects.clear().  Why do we need to iterate it here and
> manually clear each entry?
> 
> Another question: why do we need this m_delayedReleaseRecursionCount?  Given
> that releaseDelayedReleasedObjects() is only called just before the
> outermost JSLock is unlocked, and on heap destruction, there should be no
> chance of m_delayedReleaseRecursionCount ever being more than 0.  Am I
> missing something?

Yes.  As we clear all the delayed release objects, the release of that object could invoke some arbitrary JS that creates more delayed objects to be released.  This means that we can't clear the list with Vector::clear() and we will be in the middle of the clear, but recursively we'll be appending and then clearing as that recursive call executes the same path.  That is why we also have a recursive count so that we are only releasing at the first call and not any nested calls.  testapi.mm hits this case, see EvilAllocationObject.

That's why this section is in the ChangeLog:
    We do need to guard against the case that releasing an object can create more objects
    by calling into JS.  This case is acually tested by testapi.mm.

I'll add more to it.

> > Source/JavaScriptCore/heap/MarkedAllocator.cpp:132
> > -    // Due to the DelayedReleaseScope in tryAllocateHelper, some other thread might have
> > +    // Due to the DelayedReleaseList in tryAllocateHelper, some other thread might have
> >      // created a new block after we thought we didn't find any free cells. 
> >      while (!result && m_currentBlock) {
> >          // A new block was added by another thread so try popping the free list.
> 
> I think this while loop is now obsolete, and this comment is invalid.  Since
> we no longer have a DelayedReleaseScope in tryAllocateHelper(), there's now
> no opportunity for another thread to run and create the new block that we're
> checking for here.  Hence, this loop and comment should be removed.

I think these can be removed.  I'll remove these and rerun the tests.

[Mark Lam]

Comment on attachment 246072 [details]
Patch

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

r=me with remaining changes and tests passing.

>>>>> Source/JavaScriptCore/ChangeLog:7
>>>>> +
>>>> 
>>>> Your ChangeLog comment describes what was done but not why.  I think your description of the issue in bugzilla did a good job of explaining what the issue is.  Would you mind adding it here before you start explaining how it is fixed?
>>> 
>>> I added this paragraph to the top of the ChangeLog entry:
>>>     The root issue is that one thread, while allocating JS objects may need to garbage collect.
>>>     During that garbage collection, if there are delayed Objective C objects that need to be
>>>     released, we drop the locks to release them.  While the locks are release by that thread,
>>>     another thread can enter the VM and might have exactly the same source and therefore hit
>>>     this issue.  This fixes the problem by not dropping the locks during garbage collection.
>> 
>> I feel that the detail about the code cache having a partially initialized entry is important to understanding this issue.  How about ...
>> 
>>     The root issue is that one thread, while parsing source code into an UnlinkedCodeBlock, may need to garbage collect. During that garbage collection, if there are delayed Objective C objects that need to be released, we drop the locks to release them. While the locks are release by that thread, another thread can enter the VM and might parse exactly the same source and therefore hit this issue where the CodeCache contains an incomplete entry for that source from the first thread. This fixes the problem by not dropping the locks during garbage collection.
> 
> During GC, we used to drop all lock even if there aren't any delayed Objective C objects that need to be released.
> 
> How about:
> 
>     The issue for this bug is that one thread, takes a cache miss in CodeCache::getGlobalCodeBlock,
>     but in the process creates a cache entry with a nullptr UnlinkedCodeBlockType* which it will
>     fill in later in the function.  During the body of that function, it allocates objects  that
>     may garbage collect.  During that garbage collection, we drop the all locks.  While the 
>     locks are released by the first thread, another thread can enter the VM and might have exactly
>     the same source and enter CodeCache::getGlobalCodeBlock() itself.  When it looks up the code block,
>     it sees it as a cache it and uses the nullptr UnlinkedCodeBlockType* and crashes.  This fixes
>     the problem by not dropping the locks during garbage collection.  There are other likely scenarios
>     where we have a date structure like this code cache in an unsafe state for arbitrary reentrance.

Looks good.  typo: "date" ==> "data".

>>> Source/JavaScriptCore/heap/Heap.cpp:376
>>> +    }
>> 
>> In the original ~DelayedReleaseScope(), we do the release by simply calling m_delayedReleaseObjects.clear().  Why do we need to iterate it here and manually clear each entry?
>> 
>> Another question: why do we need this m_delayedReleaseRecursionCount?  Given that releaseDelayedReleasedObjects() is only called just before the outermost JSLock is unlocked, and on heap destruction, there should be no chance of m_delayedReleaseRecursionCount ever being more than 0.  Am I missing something?
> 
> Yes.  As we clear all the delayed release objects, the release of that object could invoke some arbitrary JS that creates more delayed objects to be released.  This means that we can't clear the list with Vector::clear() and we will be in the middle of the clear, but recursively we'll be appending and then clearing as that recursive call executes the same path.  That is why we also have a recursive count so that we are only releasing at the first call and not any nested calls.  testapi.mm hits this case, see EvilAllocationObject.
> 
> That's why this section is in the ChangeLog:
>     We do need to guard against the case that releasing an object can create more objects
>     by calling into JS.  This case is acually tested by testapi.mm.
> 
> I'll add more to it.

I see.  That's clear now.  Can you put that comment in this block of code since it is not automatically clear from just reading the code as is?

[Michael Saboff]

Committed r179728: <http://trac.webkit.org/changeset/179728>

[Geoffrey Garen]

Comment on attachment 246072 [details]
Patch

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

> Source/JavaScriptCore/heap/Heap.cpp:374
> +            RetainPtr<CFTypeRef> objectToRelease = m_delayedReleaseObjects.takeLast();
> +            objectToRelease.clear();

I agree with Mark that you should have added a comment here. Please do that.

Also, there's no need for an explicit temporary: You can just takeLast().

> Source/JavaScriptCore/runtime/JSLock.cpp:175
> +        m_vm->heap.releaseDelayedReleasedObjects();

This looks wrong. If you release ObjC objects *before* releasing the lock, then all dealloc methods will run with the lock still held, and may deadlock with other running threads. The main goal of the code you removed was to avoid that kind of deadlock. So, I think you've introduced a regression where a dealloc method can now deadlock if it waits on a lock owned by a thread waiting to run JavaScript.

A general rule in our API is that we never call out to client code while holding the lock. In fact, releaseDelayedObjectsNow() should ASSERT that the lock is not held, like so:

(1) ASSERT that lock is not held
(2) Acquire lock
(3) Increment recursion counter and do recursion check
(4) Take all items into temporary vector
(5) Release lock
(6) If temporary vector is empty, return
(7) Clear temporary vector
(8) Goto (1)

[Geoffrey Garen]

> (1) ASSERT that lock is not held

...ASSERT that lock is not held *by current thread* (other threads are OK)

[Michael Saboff]

Reopened to address ggaren's concerns.

[Michael Saboff]

(In reply to comment #9)

> A general rule in our API is that we never call out to client code while
> holding the lock. In fact, releaseDelayedObjectsNow() should ASSERT that the
> lock is not held, like so:
> 
> (1) ASSERT that lock is not by current thread held
> (2) Acquire lock
> (3) Increment recursion counter and do recursion check
> (4) Take all items into temporary vector
> (5) Release lock
> (6) If temporary vector is empty, return
> (7) Clear temporary vector
> (8) Goto (1)

Tried implementing these steps and there is a problem with infinite recursion.  The first time through, you proceed through steps 1-5.  When the lock is released at step 5, we reenter performing steps 1, 2 and 3 at which point we realize that we have entered recursively.  In the process of exiting, we drop the lock and reenter.

I have implemented a variation where by we enter holding the lock.
(1) Increment recursion counter and do recursion check and exit if recursing
(2) ASSERT that lock is held by current thread
(3) Take all items into temporary Vector
(4) If temporary Vector is empty, return
(5) Release lock
(6) Clear temporary vector
(7) Reaquire lock
(8) Goto (2)

Not only is this recursive safe, but it satisfies the requirement that we release objects while not holding the lock.

I also added the requested comments.

[Michael Saboff]

Created attachment 247495 [details]
Updated patch.

[Benjamin Poulain]

Comment on attachment 247495 [details]
Updated patch.

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

> Source/JavaScriptCore/heap/Heap.cpp:394
> +            Vector<RetainPtr<CFTypeRef>> objectsToRelease;
> +            objectsToRelease.swap(m_delayedReleaseObjects);

objectsToRelease = WTF::move(m_delayedReleaseObjects)

> Source/JavaScriptCore/heap/Heap.cpp:404
> +                while (!objectsToRelease.isEmpty())
> +                    objectsToRelease.takeLast().clear();

If this code is hot: takeLast() is costly, especially if you chain them. Here you could loop over the whole array, clear every retain ptr, then the whole array backbuffer would be cleared when getting out of scope.

[Michael Saboff]

Created attachment 247499 [details]
Updated with Benjamin's suggestions.

[Geoffrey Garen]

Comment on attachment 247499 [details]
Updated with Benjamin's suggestions.

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

> Source/JavaScriptCore/heap/Heap.cpp:390
> +        while (1) {

Can this be "while (m_delayedReleaseObjects.size())"?

> Source/JavaScriptCore/heap/Heap.cpp:394
> +            Vector<RetainPtr<CFTypeRef>> objectsToRelease;
> +            objectsToRelease = WTF::move(m_delayedReleaseObjects);

Can this be one line instead of two?

[Geoffrey Garen]

Side questions:

(1) Can you update the title of this bug to reflect what's actually going on? We don't expect the CodeCache to be thread-safe -- or any other data structure in JSC -- so it's confusing to have a but title about CodeCache not being thread-safe.

(2) Can you turn your test case into a regression test?

[Michael Saboff]

(In reply to comment #16)
> Comment on attachment 247499 [details]
> Updated with Benjamin's suggestions.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247499&action=review
> 
> > Source/JavaScriptCore/heap/Heap.cpp:390
> > +        while (1) {
> 
> Can this be "while (m_delayedReleaseObjects.size())"?
> 
> > Source/JavaScriptCore/heap/Heap.cpp:394
> > +            Vector<RetainPtr<CFTypeRef>> objectsToRelease;
> > +            objectsToRelease = WTF::move(m_delayedReleaseObjects);
> 
> Can this be one line instead of two?

Yes on both counts.  

I prefer "while (!m_delayedReleaseObjects.empty())".

Done in my source tree.

[Michael Saboff]

(In reply to comment #17)
> Side questions:
> 
> (1) Can you update the title of this bug to reflect what's actually going
> on? We don't expect the CodeCache to be thread-safe -- or any other data
> structure in JSC -- so it's confusing to have a but title about CodeCache
> not being thread-safe.

How about "DelayedReleaseScope drops locks during GC which can cause a thread switch."

> (2) Can you turn your test case into a regression test?

This is already tested in the testApi "EvilAllocationObject" test case.

[Michael Saboff]

Created attachment 247820 [details]
Updated patch with regression test

The Objective-C++ anonymous block functions declarations fail the style checker.

[WebKit Commit Bot]

Attachment 247820 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:133:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:144:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:157:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:167:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:175:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:236:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:246:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:298:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:339:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:359:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:367:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:371:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:375:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 13 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 247820 [details]
Updated patch with regression test

r=me

[Michael Saboff]

Created attachment 247824 [details]
Patch for landing with speculative fix for pre 10.10 Mac OS X builds

[WebKit Commit Bot]

Attachment 247824 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:132:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:143:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:166:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:174:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:235:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:245:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:297:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:338:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:358:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:366:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:370:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/Regress141275.mm:374:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 13 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Committed r180992: <http://trac.webkit.org/changeset/180992>