Bug 212069

As someone building a game, I would love to see Safari/WebKit re-enable SharedArrayBuffer. Sharing data between web workers and the main thread without SharedArrayBuffer is much slower, even though Safari is faster at a lot of things than other browsers.

*** Bug 197191 has been marked as a duplicate of this bug. ***

*** Bug 185038 has been marked as a duplicate of this bug. ***

Created attachment 413257 [details]
Patch

Yusuke, I might be missing something but it doesn't seem like this patch guards it with cross-origin isolated (as defined in the HTML Standard). That's somewhat important if you want to enable this in a safe manner.

Cross-origin isolated is https://bugs.webkit.org/show_bug.cgi?id=215407 - as Anne said, I believe it should be blocking this bug.

Created attachment 413275 [details]
Patch

(In reply to Anne van Kesteren from comment #5)
> Yusuke, I might be missing something but it doesn't seem like this patch
> guards it with cross-origin isolated (as defined in the HTML Standard).
> That's somewhat important if you want to enable this in a safe manner.

(In reply to Simon Pieters from comment #6)
> Cross-origin isolated is https://bugs.webkit.org/show_bug.cgi?id=215407 - as
> Anne said, I believe it should be blocking this bug.

We will implement it, and for SharedArrayBuffer / Atomics, we will enable it for WebKitTestRunner etc.
Once cross-origin isolation is done, we just flip runtime flag to expose.

And possibly we will enable it for JavaScriptCore.framework.

Created attachment 413388 [details]
Patch

Created attachment 413398 [details]
Patch

Created attachment 413408 [details]
Patch

Thanks for confirming Yusuke, exciting!

Comment on attachment 413408 [details]
Patch

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

r=me with some nits, if you fix the style issues.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2725
> +
> +            if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadIndexingType)
> +                || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadConstantCache)
> +                || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCache)
> +                || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadType))
> +                return false;

Is there a reason to not include all exits (other than Uncountable and friends) here?

> Source/JavaScriptCore/jsc.cpp:3102
> +    fprintf(stderr, "  --can-block-is-false       Make main thread's [[CanBlock]] false\n");

Nit: Maybe phrase this as "Make main thread's Atomics.wait throw"? Can block may be a little obscure and I'm sure I'll forget in the future...

> Tools/Scripts/test262/Runner.pm:708
> +    if (grep $_ eq 'CanBlockIsFalse', @{$data->{flags}}) {
> +        $featureFlags .= ' --can-block-is-false';
> +    }

Yikes, that seems expensive to do on every file...

Comment on attachment 413408 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2725
>> +                return false;
> 
> Is there a reason to not include all exits (other than Uncountable and friends) here?

Atomic operation is GetByVal from TypedArray + read-modify-write operation. And it also has Int32 Type checks for index argument.
And we also need FixupPhase's checkArray thing to ensure that input is requested TypedArray.
Those checks may emit these exits. And I need to include OutOfBounds too here. Changed.

>> Source/JavaScriptCore/jsc.cpp:3102
>> +    fprintf(stderr, "  --can-block-is-false       Make main thread's [[CanBlock]] false\n");
> 
> Nit: Maybe phrase this as "Make main thread's Atomics.wait throw"? Can block may be a little obscure and I'm sure I'll forget in the future...

Nice, fixed.

>> Tools/Scripts/test262/Runner.pm:708
>> +    }
> 
> Yikes, that seems expensive to do on every file...

We are already doing flags check to extract `noStrict` etc. So, I don't expect this will change the cost :)

Committed r269531: <https://trac.webkit.org/changeset/269531>

<rdar://problem/71129125>