Summary: | DFG JIT should inline Math.min, Math.max, and Math.sqrt | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | rniwa, sam | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Filip Pizlo
2011-09-17 17:19:14 PDT
Created attachment 107780 [details]
the patch
Comment on attachment 107780 [details]
the patch
This looks really great. It's a shame that the intrinsics are in addition to the "thunk generators". The old thunks are really for intrinsics. It would probably be cleaner to remove the ThunkGenerators from the lookup table, and just have an enum value. When native function objects are constructed the thunk creation could just switch on the intrinsic enum value to select a generator, where one exists (and maybe over time these may be deprecated, as hot cases are inlined in the DFG JIT).
(In reply to comment #2) > (From update of attachment 107780 [details]) > This looks really great. It's a shame that the intrinsics are in addition to the "thunk generators". The old thunks are really for intrinsics. It would probably be cleaner to remove the ThunkGenerators from the lookup table, and just have an enum value. When native function objects are constructed the thunk creation could just switch on the intrinsic enum value to select a generator, where one exists (and maybe over time these may be deprecated, as hot cases are inlined in the DFG JIT). Agree. Bug created: https://bugs.webkit.org/show_bug.cgi?id=68328 It appears that this patch introduced some regression in JSC. I can no longer load internal builds of Google docs. It repeatedly encounters the following error in WebKit2 (regardless of whether I use single process or not): ERROR: Failed to send message to real exception port, error 10000003 /Volumes/Data/webkit/Source/WebKit2/Platform/CoreIPC/mac/ConnectionMac.cpp(407) : void CoreIPC::Connection::exceptionSourceEventHandler() I've manually bisected the builds and r95397 worked fine so I'm pretty certain this patch caused it. Unfortunately, I don't know ways to provide access to our internal builds of Google docs at the moment :( so we can only inspect the code change manually. (In reply to comment #5) > It appears that this patch introduced some regression in JSC. I can no longer load internal builds of Google docs. It repeatedly encounters the following error in WebKit2 (regardless of whether I use single process or not): > > ERROR: Failed to send message to real exception port, error 10000003 > /Volumes/Data/webkit/Source/WebKit2/Platform/CoreIPC/mac/ConnectionMac.cpp(407) : void CoreIPC::Connection::exceptionSourceEventHandler() > > I've manually bisected the builds and r95397 worked fine so I'm pretty certain this patch caused it. Unfortunately, I don't know ways to provide access to our internal builds of Google docs at the moment :( so we can only inspect the code change manually. Here's a simple test you could do to see if and how this changeset affects the problem you're seeing: Go to Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp Replace the contents of handleIntrinsic() method with just "return false". Check if you still see the problem. If it makes the problem go away, then it's highly likely the fault of this patch. If the problem persists, then it's highly unlikely that this patch caused it. Thanks for your help. I understand this can be complicated when the only repro is in internal code! (In reply to comment #6) > Here's a simple test you could do to see if and how this changeset affects the problem you're seeing: > > Go to Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp > > Replace the contents of handleIntrinsic() method with just "return false". I did, and the problem disappeared. I also double-checked that reverting the change will introduce the bug again. > Thanks for your help. I understand this can be complicated when the only repro is in internal code! Nope. I just want to help Google products don't suck on Safari :) Is there any information I can provide you to help investigating the issue? (In reply to comment #7) > (In reply to comment #6) > > Here's a simple test you could do to see if and how this changeset affects the problem you're seeing: > > > > Go to Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp > > > > Replace the contents of handleIntrinsic() method with just "return false". > > I did, and the problem disappeared. I also double-checked that reverting the change will introduce the bug again. > > > Thanks for your help. I understand this can be complicated when the only repro is in internal code! > > Nope. I just want to help Google products don't suck on Safari :) > > Is there any information I can provide you to help investigating the issue? Thanks for checking this. I'll try to track this down by increasing test coverage on our end. I will let you know if I need more info from you! |