Bug 156821 – Lets do less locking of symbol tables in the BytecodeGenerator where we don't have race conditions

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

[Saam Barati]

...

[Filip Pizlo]

I've always wanted a mode of LockHolder that says "shut up I know I don't need a lock here", to pass to methods that need a LockHolder& when I know that I own the object.  It could lead to nice syntax:

foo->something(LockHolder::DANGER_NO_LOCKING, ...);

I don't know if that applies in this case, but this could be broadly useful. It would be OK to call this in cases where you know that you just allocated 'foo' and so it hasn't escaped into the "global" heap yet.

[Saam Barati]

Created attachment 276939 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:864:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3286:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 6 files


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

[Filip Pizlo]

Comment on attachment 276939 [details]
patch

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

> Source/JavaScriptCore/runtime/ConcurrentJITLock.h:120
> +    enum NoLockingNecessaryTag { NoLockingNecessary };
> +    ConcurrentJITLocker(NoLockingNecessaryTag)
> +        : ConcurrentJITLockerBase(nullptr)
> +    {
> +    }
> +

Can you move this to WTF::Locker?

[Geoffrey Garen]

Comment on attachment 276939 [details]
patch

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

> Source/JavaScriptCore/runtime/ConcurrentJITLock.h:119
> +    enum NoLockingNecessaryTag { NoLockingNecessary };
> +    ConcurrentJITLocker(NoLockingNecessaryTag)
> +        : ConcurrentJITLockerBase(nullptr)
> +    {
> +    }

Can we assert here that the lock is not held?

[Filip Pizlo]

(In reply to comment #5)
> Comment on attachment 276939 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276939&action=review
> 
> > Source/JavaScriptCore/runtime/ConcurrentJITLock.h:119
> > +    enum NoLockingNecessaryTag { NoLockingNecessary };
> > +    ConcurrentJITLocker(NoLockingNecessaryTag)
> > +        : ConcurrentJITLockerBase(nullptr)
> > +    {
> > +    }
> 
> Can we assert here that the lock is not held?

When we talked about this earlier we meant for it to be useful in any context where you know that it's not meaningful to even speak of the lock.  You may have an object A that normally gets synchronized using a lock in object B, where object B owns object A.  You could use this version of the constructor in a code path that runs before you have even allocated object B, for example because you give object A to object B when you create object B.  To support those idioms, you wouldn't want to have to even mention the lock here.

[Saam Barati]

Created attachment 276942 [details]
patch

Added to WTF::Locker too.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:864:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3286:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Locker.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 8 files


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

[Build Bot]

Comment on attachment 276942 [details]
patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 276947 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Saam Barati]

Created attachment 276956 [details]
patch for landing

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:864:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3286:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Locker.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 8 files


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

[Saam Barati]

Created attachment 276962 [details]
patch for landing

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:864:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3286:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Locker.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 8 files


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

[WebKit Commit Bot]

Comment on attachment 276962 [details]
patch for landing

Clearing flags on attachment: 276962

Committed r199848: <http://trac.webkit.org/changeset/199848>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.