Bug 144457 – Use jsTypeofIsObject() in DFG AI and operationTypeOfIsObject()

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

[Filip Pizlo]

This would allow it to fold IsFunction(exotic object constant) and IsObjectOrNull(exotic object constant).

[Alexey Shvayka]

Created attachment 406897 [details]
Patch

[Saam Barati]

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.

[Saam Barati]

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*

[Alexey Shvayka]

Created attachment 406950 [details]
Patch

Break jsTypeofIsObject() and jsTypeofIsFunction() into 2 functions (each).

[Alexey Shvayka]

(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).

[Alexey Shvayka]

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.

[Saam Barati]

(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?

[Alexey Shvayka]

(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.

[EWS]

Committed r265965: <https://trac.webkit.org/changeset/265965>

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

[Darin Adler]

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));

[Radar WebKit Bug Importer]

<rdar://problem/67508764>

[Saam Barati]

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

[Darin Adler]

(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!

[Saam Barati]

(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 ||

[Yusuke Suzuki]

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...?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 215744

[Alexey Shvayka]

Created attachment 407254 [details]
Patch

[Alexey Shvayka]

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?

[Yusuke Suzuki]

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.

[Alexey Shvayka]

Created attachment 407318 [details]
Patch

Introduce invert(TriState).

[EWS]

Committed r266223: <https://trac.webkit.org/changeset/266223>

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