Bug 122667 – llint_slow_path_put_by_id can deadlock on a ConcurrentJITLock

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

[Mark Hahnenberg]

llint_slow_path_put_by_id has the following line of code:

   599 	                ConcurrentJITLocker locker(codeBlock->m_lock);

and later in the same scope:

   618 	                        StructureChain* chain = structure->prototypeChain(exec);

which can cause a garbage collection. Garbage collection acquires the ConcurrentJITLock as it processes CodeBlocks, causing a deadlock.

[Filip Pizlo]

(In reply to comment #0)
> llint_slow_path_put_by_id has the following line of code:
> 
>    599                     ConcurrentJITLocker locker(codeBlock->m_lock);
> 
> and later in the same scope:
> 
>    618                             StructureChain* chain = structure->prototypeChain(exec);
> 
> which can cause a garbage collection. Garbage collection acquires the ConcurrentJITLock as it processes CodeBlocks, causing a deadlock.

L

[Filip Pizlo]

(In reply to comment #1)
> (In reply to comment #0)
> > llint_slow_path_put_by_id has the following line of code:
> > 
> >    599                     ConcurrentJITLocker locker(codeBlock->m_lock);
> > 
> > and later in the same scope:
> > 
> >    618                             StructureChain* chain = structure->prototypeChain(exec);
> > 
> > which can cause a garbage collection. Garbage collection acquires the ConcurrentJITLock as it processes CodeBlocks, causing a deadlock.
> 
> L

Dunno what happened there.  I meant to say "Lol."

[Filip Pizlo]

(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > llint_slow_path_put_by_id has the following line of code:
> > > 
> > >    599                     ConcurrentJITLocker locker(codeBlock->m_lock);
> > > 
> > > and later in the same scope:
> > > 
> > >    618                             StructureChain* chain = structure->prototypeChain(exec);
> > > 
> > > which can cause a garbage collection. Garbage collection acquires the ConcurrentJITLock as it processes CodeBlocks, causing a deadlock.
> > 
> > L
> 
> Dunno what happened there.  I meant to say "Lol."

We could either move the call to prototypeChain(exec) up above the ConcurrentJITLocker, or we could do my favorite thing: DeferGC.

I like DeferGC more since it's more comprehensive.

[Mark Hahnenberg]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > (In reply to comment #0)
> > > > llint_slow_path_put_by_id has the following line of code:
> > > > 
> > > >    599                     ConcurrentJITLocker locker(codeBlock->m_lock);
> > > > 
> > > > and later in the same scope:
> > > > 
> > > >    618                             StructureChain* chain = structure->prototypeChain(exec);
> > > > 
> > > > which can cause a garbage collection. Garbage collection acquires the ConcurrentJITLock as it processes CodeBlocks, causing a deadlock.
> > > 
> > > L
> > 
> > Dunno what happened there.  I meant to say "Lol."
> 
> We could either move the call to prototypeChain(exec) up above the ConcurrentJITLocker, or we could do my favorite thing: DeferGC.
> 
> I like DeferGC more since it's more comprehensive.

Yeah I like that too.

[Mark Hahnenberg]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (In reply to comment #1)
> > > > (In reply to comment #0)
> > > > > llint_slow_path_put_by_id has the following line of code:
> > > > > 
> > > > >    599                     ConcurrentJITLocker locker(codeBlock->m_lock);
> > > > > 
> > > > > and later in the same scope:
> > > > > 
> > > > >    618                             StructureChain* chain = structure->prototypeChain(exec);
> > > > > 
> > > > > which can cause a garbage collection. Garbage collection acquires the ConcurrentJITLock as it processes CodeBlocks, causing a deadlock.
> > > > 
> > > > L
> > > 
> > > Dunno what happened there.  I meant to say "Lol."
> > 
> > We could either move the call to prototypeChain(exec) up above the ConcurrentJITLocker, or we could do my favorite thing: DeferGC.
> > 
> > I like DeferGC more since it's more comprehensive.
> 
> Yeah I like that too.

We could even make ConcurrentJITLocker do an implicit DeferGC. Thoughts?

[Filip Pizlo]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > (In reply to comment #1)
> > > > > (In reply to comment #0)
> > > > > > llint_slow_path_put_by_id has the following line of code:
> > > > > > 
> > > > > >    599                     ConcurrentJITLocker locker(codeBlock->m_lock);
> > > > > > 
> > > > > > and later in the same scope:
> > > > > > 
> > > > > >    618                             StructureChain* chain = structure->prototypeChain(exec);
> > > > > > 
> > > > > > which can cause a garbage collection. Garbage collection acquires the ConcurrentJITLock as it processes CodeBlocks, causing a deadlock.
> > > > > 
> > > > > L
> > > > 
> > > > Dunno what happened there.  I meant to say "Lol."
> > > 
> > > We could either move the call to prototypeChain(exec) up above the ConcurrentJITLocker, or we could do my favorite thing: DeferGC.
> > > 
> > > I like DeferGC more since it's more comprehensive.
> > 
> > Yeah I like that too.
> 
> We could even make ConcurrentJITLocker do an implicit DeferGC. Thoughts?

Or even better: have two versions of ConcurrentJITLocker.

One version sets a thread-local variable that in causes all GC allocations and other GC calls to fail in debug mode.  We could use that in as many places as possible.  The cool thing about this is that it also guarantees that the method is safe to call from a concurrent thread, from the GC's standpoint.

Another version does what you say.

We can then try to use the asserting version in as many places as possible.  Probably by doing this we will learn some fun surprises.

[Mark Hahnenberg]

Created attachment 214030 [details]
Patch

[Filip Pizlo]

Comment on attachment 214030 [details]
Patch

love it.

[EFL EWS Bot]

Comment on attachment 214030 [details]
Patch

Attachment 214030 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3950221

[Mark Hahnenberg]

Created attachment 214031 [details]
build fix

[EFL EWS Bot]

Comment on attachment 214031 [details]
build fix

Attachment 214031 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3776306

[EFL EWS Bot]

Comment on attachment 214031 [details]
build fix

Attachment 214031 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3946253

[Mark Hahnenberg]

Created attachment 214035 [details]
omgjustbuild

[WebKit Commit Bot]

Comment on attachment 214035 [details]
omgjustbuild

Rejecting attachment 214035 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 214035, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
avaScriptCore/llint/LLIntSlowPaths.cpp
patching file Source/JavaScriptCore/runtime/ConcurrentJITLock.h
patching file Source/JavaScriptCore/runtime/InitializeThreading.cpp
patching file Source/JavaScriptCore/runtime/JSCellInlines.h
patching file Source/JavaScriptCore/runtime/Structure.cpp
patching file Source/JavaScriptCore/runtime/Structure.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Filip Pizlo']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/3766906

[Mark Hahnenberg]

Committed r157413: <http://trac.webkit.org/changeset/157413>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 122779

[Alexey Proskuryakov]

This looks like the most likely culprit for WebGL and other crashes and timeouts:

http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r157416%20(10770)/results.html

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r157419%20(13809)/results.html

Rolled out in <https://trac.webkit.org/r157424>. We'll see if this helps.

[Ryosuke Niwa]

This may have improved Dromaeo/jslib-event-jquery's score by ~28%:
https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-lion%22%2C%22Dromaeo%2Fjslib-event-jquery%3ARuns%22%5D%2C%5B%22mac-mountainlion%22%2C%22Dromaeo%2Fjslib-event-jquery%3ARuns%22%5D%2C%5B%22mac-mountainlion%22%2C%22Dromaeo%2Fjslib-event-prototype%3ARuns%22%5D%2C%5B%22mac-lion%22%2C%22Dromaeo%2Fjslib-event-prototype%3ARuns%22%5D%5D&days=52&zoom=%5B1381585431357.5227%2C1381827392596%5D

[Mark Hahnenberg]

(In reply to comment #18)
> This may have improved Dromaeo/jslib-event-jquery's score by ~28%:
> https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-lion%22%2C%22Dromaeo%2Fjslib-event-jquery%3ARuns%22%5D%2C%5B%22mac-mountainlion%22%2C%22Dromaeo%2Fjslib-event-jquery%3ARuns%22%5D%2C%5B%22mac-mountainlion%22%2C%22Dromaeo%2Fjslib-event-prototype%3ARuns%22%5D%2C%5B%22mac-lion%22%2C%22Dromaeo%2Fjslib-event-prototype%3ARuns%22%5D%5D&days=52&zoom=%5B1381585431357.5227%2C1381827392596%5D

Uhh, I don't think so. The patch was rolled out, but we still have the perf win. Also, this patch just fixes a deadlock, no reason it would improve performance.

[Ryosuke Niwa]

So it's probably either http://trac.webkit.org/changeset/157412 or http://trac.webkit.org/changeset/157411

[Mark Hahnenberg]

(In reply to comment #20)
> So it's probably either http://trac.webkit.org/changeset/157412 or http://trac.webkit.org/changeset/157411

Yeah, my money is on http://trac.webkit.org/changeset/157411.

[Mark Hahnenberg]

Created attachment 214399 [details]
Patch

[Geoffrey Garen]

Comment on attachment 214399 [details]
Patch

r=me based on previous reviews, assuming tests pass.

[Mark Hahnenberg]

Committed r157539: <http://trac.webkit.org/changeset/157539>