We should remove this, which is virtual call and it resides in performance critical path (putDirectInternal).
Note that reifyPropertyNameIfNeeded is only used by JSFunction.
Created attachment 340081 [details] Patch
Created attachment 340082 [details] Patch
Created attachment 340083 [details] Patch
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 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 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 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).
Created attachment 340170 [details] Patch
It seems like ErrorInstance also reifies stuff.
(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.
(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.
Ping?
Created attachment 340482 [details] Patch
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
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 on attachment 340482 [details] Patch r=me
Committed r231902: <https://trac.webkit.org/changeset/231902>
<rdar://problem/40332731>
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
(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.
Committed r231957: <https://trac.webkit.org/changeset/231957>
Committed r231976: <https://trac.webkit.org/changeset/231976>