Bug 68318 – DFG JIT should inline Math.min, Math.max, and Math.sqrt

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

[Filip Pizlo]

The Math.min, Math.max, and Math.sqrt functions are all trivial to inline as they do not require calls to C helpers.  DFG JIT should inline them.

[Filip Pizlo]

Created attachment 107780 [details]
the patch

[Gavin Barraclough]

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

[Filip Pizlo]

Landed in r95399.

[Filip Pizlo]

(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

[Ryosuke Niwa]

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.

[Filip Pizlo]

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

[Ryosuke Niwa]

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

[Filip Pizlo]

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