Bug 185350 – [JSC] Remove reifyPropertyNameIfNeeded

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

[Yusuke Suzuki]

We should remove this, which is virtual call and it resides in performance critical path (putDirectInternal).

[Yusuke Suzuki]

Note that reifyPropertyNameIfNeeded is only used by JSFunction.

[Yusuke Suzuki]

Created attachment 340081 [details]
Patch

[Yusuke Suzuki]

Created attachment 340082 [details]
Patch

[Yusuke Suzuki]

Created attachment 340083 [details]
Patch

[Saam Barati]

Comment on attachment 340083 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        reifyPropertyNameIfNeeded is in the middle of putDirectInternal, which is super critical path.

What was the original bug we were fixing with this again? Wasn't it just with setters/getters? Maybe let's just call it there? I'm really not a fan of this MethodTable method since it's super half baked.

[Saam Barati]

Comment on attachment 340083 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        reifyPropertyNameIfNeeded is in the middle of putDirectInternal, which is super critical path.
> 
> What was the original bug we were fixing with this again? Wasn't it just with setters/getters? Maybe let's just call it there? I'm really not a fan of this MethodTable method since it's super half baked.

Hah. I see your patch does that.

[Saam Barati]

Comment on attachment 340083 [details]
Patch

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.h:228
> +enum class RequireCacheStructreMode { Yes, No };

Since you're inlining this already, and you have the ability to templatize it, it feels like a more canonical way to do this would be to take a Structure** as a final parameter. And just default that to Structure**=nullptr. Since it's inlined, the compiler should remove the branch.

> Source/JavaScriptCore/runtime/CommonSlowPaths.h:232
> +    if (baseObject->inherits<JSFunction>(vm)) {

Do we really not use reification in other classes?

[Yusuke Suzuki]

Comment on attachment 340083 [details]
Patch

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

>> Source/JavaScriptCore/runtime/CommonSlowPaths.h:228
>> +enum class RequireCacheStructreMode { Yes, No };
> 
> Since you're inlining this already, and you have the ability to templatize it, it feels like a more canonical way to do this would be to take a Structure** as a final parameter. And just default that to Structure**=nullptr. Since it's inlined, the compiler should remove the branch.

Sounds nice. Fixed.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.h:232
>> +    if (baseObject->inherits<JSFunction>(vm)) {
> 
> Do we really not use reification in other classes?

Right now, we do not have any `reifyPropertyNameIfNeeded` functions except for JSFunction. So this patch does not break the things.
If we have another class which should needs reification, I think we should make this JSTypeInfo flag checking here (And add type flag).

[Yusuke Suzuki]

Created attachment 340170 [details]
Patch

[Saam Barati]

It seems like ErrorInstance also reifies stuff.

[Saam Barati]

(In reply to Saam Barati from comment #10)
> It seems like ErrorInstance also reifies stuff.

I'm not sure it can run into whatever bug JF found originally or not.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #11)
> (In reply to Saam Barati from comment #10)
> > It seems like ErrorInstance also reifies stuff.
> 
> I'm not sure it can run into whatever bug JF found originally or not.

I think this is OK. These functions are used for operations, which are invoked by bytecodes. And these bytecodes should be used for

- object literals
- class literals

In object literals, the target is JSFinalObject.
And in class literals with static functions, the target is a constructor, which is JSFunction.

When we use putDirect for the other types of objects from operations, we do not accept arbitrary values as its inputs and property names.

So, here, I think handling JSFunction is enough.

[Yusuke Suzuki]

Ping?

[Yusuke Suzuki]

Created attachment 340482 [details]
Patch

[EWS Watchlist]

Comment on attachment 340482 [details]
Patch

Attachment 340482 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7699886

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 340515 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Yusuke Suzuki]

Ping?

[Saam Barati]

Comment on attachment 340482 [details]
Patch

r=me

[Yusuke Suzuki]

Committed r231902: <https://trac.webkit.org/changeset/231902>

[Radar WebKit Bug Importer]

<rdar://problem/40332731>

[Ryan Haddad]

This change introduced crashes on JSC Debug bots due to an unchecked exception:

ERROR: Unchecked JS exception:
    This scope can throw a JS exception: putDirectAccessorWithReify @ /Volumes/Data/slave/highsierra-32bitJSC-debug/build/Source/JavaScriptCore/runtime/CommonSlowPaths.h:243
        (ExceptionScope::m_recursionDepth was 9)
    But the exception was unchecked as of this scope: defineOwnProperty @ ./runtime/JSFunction.cpp:554
        (ExceptionScope::m_recursionDepth was 9)

https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1931

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1024

[Yusuke Suzuki]

(In reply to Ryan Haddad from comment #21)
> This change introduced crashes on JSC Debug bots due to an unchecked
> exception:
> 
> ERROR: Unchecked JS exception:
>     This scope can throw a JS exception: putDirectAccessorWithReify @
> /Volumes/Data/slave/highsierra-32bitJSC-debug/build/Source/JavaScriptCore/
> runtime/CommonSlowPaths.h:243
>         (ExceptionScope::m_recursionDepth was 9)
>     But the exception was unchecked as of this scope: defineOwnProperty @
> ./runtime/JSFunction.cpp:554
>         (ExceptionScope::m_recursionDepth was 9)
> 
> https://build.webkit.org/builders/Apple%20High%20Sierra%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/1931
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1024

I'll check this soon.

[Yusuke Suzuki]

Committed r231957: <https://trac.webkit.org/changeset/231957>

[Yusuke Suzuki]

Committed r231976: <https://trac.webkit.org/changeset/231976>