Bug 144457 - Use jsTypeofIsObject() in DFG AI and operationTypeOfIsObject()
Summary: Use jsTypeofIsObject() in DFG AI and operationTypeOfIsObject()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Enhancement
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on: 144409 215744
Blocks:
  Show dependency treegraph
 
Reported: 2015-04-30 10:32 PDT by Filip Pizlo
Modified: 2020-08-26 22:27 PDT (History)
14 users (show)

See Also:


Attachments
Patch (6.74 KB, patch)
2020-08-19 17:49 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (8.32 KB, patch)
2020-08-20 11:59 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (8.06 KB, patch)
2020-08-25 18:12 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (9.24 KB, patch)
2020-08-26 12:04 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-04-30 10:32:17 PDT
This would allow it to fold IsFunction(exotic object constant) and IsObjectOrNull(exotic object constant).
Comment 1 Alexey Shvayka 2020-08-19 17:49:53 PDT
Created attachment 406897 [details]
Patch
Comment 2 Saam Barati 2020-08-19 18:30:18 PDT
Comment on attachment 406897 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/Operations.h:38
> +ALWAYS_INLINE bool jsTypeofIsObject(JSGlobalObject* globalObject, JSValue value)

nit: Maybe break this into two functions: JSObject* and JSValue as argument implementations, and have the DFG call the JSObject* parameter version.
Comment 3 Saam Barati 2020-08-19 18:30:56 PDT
Comment on attachment 406897 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Operations.h:38
>> +ALWAYS_INLINE bool jsTypeofIsObject(JSGlobalObject* globalObject, JSValue value)
> 
> nit: Maybe break this into two functions: JSObject* and JSValue as argument implementations, and have the DFG call the JSObject* parameter version.

to specify more: the DFGOperations functions, which already has a known JSObject*
Comment 4 Alexey Shvayka 2020-08-20 11:59:15 PDT
Created attachment 406950 [details]
Patch

Break jsTypeofIsObject() and jsTypeofIsFunction() into 2 functions (each).
Comment 5 Alexey Shvayka 2020-08-20 11:59:57 PDT
(In reply to Saam Barati from comment #2)
> nit: Maybe break this into two functions: JSObject* and JSValue as argument
> implementations, and have the DFG call the JSObject* parameter version.

Thank you for review, Saam.

I've attempted to make this change in r265907 (per your earlier comment), but it was segfaulting.
TIL jsTypeofIsX(JSGlobalObject*, JSObject*) should come before TypeofIsX(JSGlobalObject*, JSValue).
Comment 6 Alexey Shvayka 2020-08-20 12:00:23 PDT
Comment on attachment 406950 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGOperations.cpp:-2112
> -    ASSERT(jsDynamicCast<JSObject*>(vm, object));

Removing ASSERT since asObject() has it.
Comment 7 Saam Barati 2020-08-20 12:28:56 PDT
(In reply to Alexey Shvayka from comment #5)
> (In reply to Saam Barati from comment #2)
> > nit: Maybe break this into two functions: JSObject* and JSValue as argument
> > implementations, and have the DFG call the JSObject* parameter version.
> 
> Thank you for review, Saam.
> 
> I've attempted to make this change in r265907 (per your earlier comment),
> but it was segfaulting.
> TIL jsTypeofIsX(JSGlobalObject*, JSObject*) should come before
> TypeofIsX(JSGlobalObject*, JSValue).

What do you mean by "come before"?

Why is it segfaulting?
Comment 8 Alexey Shvayka 2020-08-20 12:36:39 PDT
(In reply to Saam Barati from comment #7)
> What do you mean by "come before"?
> 
> Why is it segfaulting?

I meant jsTypeofIsX(JSGlobalObject*, JSObject*) should be declared before TypeofIsX(JSGlobalObject*, JSValue) in Operations.h, otherwise TypeofIsX(JSGlobalObject*, JSValue) would be calling TypeofIsX(JSGlobalObject*, JSValue) for object values over and over again, segfaulting in --debug.

All fixed now.
Comment 9 EWS 2020-08-20 14:26:02 PDT
Committed r265965: <https://trac.webkit.org/changeset/265965>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406950 [details].
Comment 10 Darin Adler 2020-08-20 15:06:55 PDT
Comment on attachment 406950 [details]
Patch

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

> Source/JavaScriptCore/runtime/Operations.h:45
> +    if (object->structure(vm)->masqueradesAsUndefined(globalObject))
> +        return false;
> +    if (object->isCallable(vm))
> +        return false;
> +    return true;

Style thought for the future in cases like this. I think this would read really nicely as a boolean expression:

    return !object->structure(vm)->masqueradesAsUndefined(globalObject) && !object->isCallable(vm);

I think it is easier to read and understand than the multiple return statements.

> Source/JavaScriptCore/runtime/Operations.h:52
> +    if (value.isObject())
> +        return jsTypeofIsObject(globalObject, asObject(value));
> +    return value.isNull();

Please consider the same kind of thing for this:

    return value.isNull() || (value.isObject() && jsTypeofIsObject(globalObject, asObject(value));

> Source/JavaScriptCore/runtime/Operations.h:62
> +    if (object->structure(vm)->masqueradesAsUndefined(globalObject))
> +        return false;
> +    if (object->isCallable(vm))
> +        return true;
> +    return false;

And for this:

    return !object->structure(vm)->masqueradesAsUndefined(globalObject) && object->isCallable(vm);

> Source/JavaScriptCore/runtime/Operations.h:69
> +    if (value.isObject())
> +        return jsTypeofIsFunction(globalObject, asObject(value));
>      return false;

And for this:

    return value.isObject() && jsTypeofIsFunction(globalObject, asObject(value));
Comment 11 Radar WebKit Bug Importer 2020-08-20 15:07:10 PDT
<rdar://problem/67508764>
Comment 12 Saam Barati 2020-08-20 17:21:00 PDT
Comment on attachment 406950 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Operations.h:45
>> +    return true;
> 
> Style thought for the future in cases like this. I think this would read really nicely as a boolean expression:
> 
>     return !object->structure(vm)->masqueradesAsUndefined(globalObject) && !object->isCallable(vm);
> 
> I think it is easier to read and understand than the multiple return statements.

I actually find, for me, that as it's written, it's much easier to understand. I frequently write code like this instead of putting it all in one expression
Comment 13 Darin Adler 2020-08-20 17:45:25 PDT
(In reply to Saam Barati from comment #12)
> I actually find, for me, that as it's written, it's much easier to
> understand. I frequently write code like this instead of putting it all in
> one expression.

I find it’s easier to *write* with the returns, but easier to *read* with the boolean expressions.

I wonder how we’ll settle this!
Comment 14 Saam Barati 2020-08-20 17:50:31 PDT
(In reply to Darin Adler from comment #13)
> (In reply to Saam Barati from comment #12)
> > I actually find, for me, that as it's written, it's much easier to
> > understand. I frequently write code like this instead of putting it all in
> > one expression.
> 
> I find it’s easier to *write* with the returns, but easier to *read* with
> the boolean expressions.
> 
> I wonder how we’ll settle this!

So after I typed this, I thought about it more. I don’t have any strict guidelines, but sometimes I find one form more readable than the other. And tend to lean towards early returns. I think it usually has to do with how many negations I’m dealing with, and if it’s combined with && or ||
Comment 15 Yusuke Suzuki 2020-08-21 15:04:56 PDT
Comment on attachment 406950 [details]
Patch

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

> Source/JavaScriptCore/runtime/Operations.h:43
> +    if (object->isCallable(vm))

I wonder if this is OK. This will invoke getCallData for non function objects. But this function can be called from the concurrent compiler too.
So, we need to ensure that getCallData implementation is OK for concurrent access.
Can you review all `getCallData` implementation and ensure this is safe for concurrent calls?
And maybe, we should rename it to `getCallDataConcurrently` to make it explicit that this is called concurrently...?
Comment 16 WebKit Commit Bot 2020-08-21 16:45:02 PDT
Re-opened since this is blocked by bug 215744
Comment 17 Alexey Shvayka 2020-08-25 18:12:25 PDT
Created attachment 407254 [details]
Patch
Comment 18 Alexey Shvayka 2020-08-25 18:14:41 PDT
Comment on attachment 407254 [details]
Patch

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

> Source/JavaScriptCore/runtime/Operations.h:46
> +    return triState(object->isCallableWithConcurrency<concurrency>(vm) == TriState::False);

We may consider overriding logical not (!) operator for TriState that flips True <=> False while preserving Indeterminate state.

(In reply to Saam Barati from comment #2)
> nit: Maybe break this into two functions: JSObject* and JSValue as argument
> implementations, and have the DFG call the JSObject* parameter version.

Given the introduction of *WithConcurrency() versions, I'm not sure we should do this as it requires adding jsTypeofIsObject(JSGlobalObject*, JSObject*) + a template method?
Comment 19 Yusuke Suzuki 2020-08-25 18:37:36 PDT
Comment on attachment 407254 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Operations.h:46
>> +    return triState(object->isCallableWithConcurrency<concurrency>(vm) == TriState::False);
> 
> We may consider overriding logical not (!) operator for TriState that flips True <=> False while preserving Indeterminate state.
> 
> (In reply to Saam Barati from comment #2)

If `object->isCallableWithConcurrency<concurrency>(vm)` returns TriState::Indeterminnate (this is possible when concurrency is ConcurrentThread), we should return TriState::Indeterminate instead of true/false.
Comment 20 Alexey Shvayka 2020-08-26 12:04:51 PDT
Created attachment 407318 [details]
Patch

Introduce invert(TriState).
Comment 21 EWS 2020-08-26 22:27:20 PDT
Committed r266223: <https://trac.webkit.org/changeset/266223>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407318 [details].