Bug 185350 - [JSC] Remove reifyPropertyNameIfNeeded
Summary: [JSC] Remove reifyPropertyNameIfNeeded
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 179734 180644
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-05 06:18 PDT by Yusuke Suzuki
Modified: 2018-05-18 11:55 PDT (History)
7 users (show)

See Also:


Attachments
Patch (27.52 KB, patch)
2018-05-10 04:40 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.60 KB, patch)
2018-05-10 04:44 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.96 KB, patch)
2018-05-10 05:06 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.27 KB, patch)
2018-05-10 22:06 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (27.50 KB, patch)
2018-05-16 02:20 PDT, Yusuke Suzuki
sbarati: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.75 MB, application/zip)
2018-05-16 12:43 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-05-05 06:18:45 PDT
We should remove this, which is virtual call and it resides in performance critical path (putDirectInternal).
Comment 1 Yusuke Suzuki 2018-05-05 06:27:18 PDT
Note that reifyPropertyNameIfNeeded is only used by JSFunction.
Comment 2 Yusuke Suzuki 2018-05-10 04:40:44 PDT
Created attachment 340081 [details]
Patch
Comment 3 Yusuke Suzuki 2018-05-10 04:44:24 PDT
Created attachment 340082 [details]
Patch
Comment 4 Yusuke Suzuki 2018-05-10 05:06:23 PDT
Created attachment 340083 [details]
Patch
Comment 5 Saam Barati 2018-05-10 21:38:18 PDT
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.
Comment 6 Saam Barati 2018-05-10 21:38:55 PDT
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.
Comment 7 Saam Barati 2018-05-10 21:42:49 PDT
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?
Comment 8 Yusuke Suzuki 2018-05-10 21:46:35 PDT
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).
Comment 9 Yusuke Suzuki 2018-05-10 22:06:35 PDT
Created attachment 340170 [details]
Patch
Comment 10 Saam Barati 2018-05-10 23:04:34 PDT
It seems like ErrorInstance also reifies stuff.
Comment 11 Saam Barati 2018-05-10 23:04:52 PDT
(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.
Comment 12 Yusuke Suzuki 2018-05-10 23:24:51 PDT
(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.
Comment 13 Yusuke Suzuki 2018-05-11 22:58:39 PDT
Ping?
Comment 14 Yusuke Suzuki 2018-05-16 02:20:40 PDT
Created attachment 340482 [details]
Patch
Comment 15 Build Bot 2018-05-16 12:43:07 PDT
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
Comment 16 Build Bot 2018-05-16 12:43:19 PDT
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
Comment 17 Yusuke Suzuki 2018-05-16 22:30:02 PDT
Ping?
Comment 18 Saam Barati 2018-05-17 08:56:32 PDT
Comment on attachment 340482 [details]
Patch

r=me
Comment 19 Yusuke Suzuki 2018-05-17 09:08:57 PDT
Committed r231902: <https://trac.webkit.org/changeset/231902>
Comment 20 Radar WebKit Bug Importer 2018-05-17 09:11:10 PDT
<rdar://problem/40332731>
Comment 21 Ryan Haddad 2018-05-17 17:02:49 PDT
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
Comment 22 Yusuke Suzuki 2018-05-18 09:12:17 PDT
(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.
Comment 23 Yusuke Suzuki 2018-05-18 09:15:55 PDT
Committed r231957: <https://trac.webkit.org/changeset/231957>
Comment 24 Yusuke Suzuki 2018-05-18 11:55:41 PDT
Committed r231976: <https://trac.webkit.org/changeset/231976>