Bug 215746 – [JSC] Add concurrency-aware version of isCallable / isConstructor to make it usable in DFG compiler

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Add concurrency-aware version of isCallable / isConstructor to make it usable in DFG compiler

[Yusuke Suzuki]

Created attachment 407035 [details]
Patch

[Yusuke Suzuki]

Created attachment 407036 [details]
Patch

[Yusuke Suzuki]

Created attachment 407037 [details]
Patch

[Saam Barati]

Comment on attachment 407037 [details]
Patch

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

> Source/JavaScriptCore/runtime/Concurrency.h:34
> +    OnMainThread,
> +    Concurrently,

to make these names match, I'd do:
OnMainThread, OnConcurrentThread

MainThread, ConcurrentThread

Main, Concurrent

> Source/JavaScriptCore/runtime/JSCellInlines.h:254
> +        else {

Do we need an else here, since previous "if constexpr" ends in a return?

> Source/JavaScriptCore/runtime/JSCellInlines.h:257
> +            if (type() == InternalFunctionType)

Maybe add a FIXME + link to bug about your plan to when NPAPI is removed b/c its concurrency issue, and rename getCallData to indicate it can be concurrent?

Or perhaps, if that doesn't work out, add a new getCallDataConcurrently version?

> Source/JavaScriptCore/runtime/JSCellInlines.h:272
> +    else {

Do we need an else here, since previous "if constexpr" ends in a return?

[Yusuke Suzuki]

Comment on attachment 407037 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/Concurrency.h:34
>> +    Concurrently,
> 
> to make these names match, I'd do:
> OnMainThread, OnConcurrentThread
> 
> MainThread, ConcurrentThread
> 
> Main, Concurrent

Changing it to MainThread / ConcurrentThread.

>> Source/JavaScriptCore/runtime/JSCellInlines.h:254
>> +        else {
> 
> Do we need an else here, since previous "if constexpr" ends in a return?

Strictly speaking, this else makes else branch `if-constexpr-else`, but in this case, the above branch returns so maybe this does not affect on the generated code. Removed.

>> Source/JavaScriptCore/runtime/JSCellInlines.h:257
>> +            if (type() == InternalFunctionType)
> 
> Maybe add a FIXME + link to bug about your plan to when NPAPI is removed b/c its concurrency issue, and rename getCallData to indicate it can be concurrent?
> 
> Or perhaps, if that doesn't work out, add a new getCallDataConcurrently version?

Added FIXME.

>> Source/JavaScriptCore/runtime/JSCellInlines.h:272
>> +    else {
> 
> Do we need an else here, since previous "if constexpr" ends in a return?

Removed because of the same reason above.

[Yusuke Suzuki]

Committed r266107: <https://trac.webkit.org/changeset/266107>

[Radar WebKit Bug Importer]

<rdar://problem/67724632>