Bug 15633 – SunSpider shows JSC spending time in KJSCHECKEXCEPTIONVALUE

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Eric Seidel (no email)]

SunSpider shows JSC spending time in KJSCHECKEXCEPTIONVALUE

I made a patch to lessen the number of ifs, there, but it seems to actually be a slowdown instead of a speedup.  Unclear why.  Perhaps we need to reverse the ifs to have the if path be the expected path.

[Eric Seidel (no email)]

Created attachment 16814 [details]
patch which makes JSC (unexpectedly) slower

[Darin Adler]

Created attachment 16815 [details]
patch I had lying around to do something similar

I haven't had a chance to performance-test this one yet.

[Eric Seidel (no email)]

Perhaps these two combined would be a win.

[Eric Seidel (no email)]

Comment on attachment 16815 [details]
patch I had lying around to do something similar

This was a speedup:

Total:                  8437.4ms [ +/- 43.52ms | +/- 0.52% ]

became:

Total:                  8322.2ms [ +/- 11.57ms | +/- 0.14% ]

[Eric Seidel (no email)]

Comment on attachment 16815 [details]
patch I had lying around to do something similar

Well, this is actually Darin's patch, so I'm going to review it. :)  Looks great!  I'll let darin do the landing honors.

[Darin Adler]

Comment on attachment 16815 [details]
patch I had lying around to do something similar

I landed this patch, so clearing the review flag.

There's still room for optimization here.

[Eric Seidel (no email)]

So the memory check is gone after the patch on bug 15658 lands.  This macro is, of course, still super hot.  I think it can safely be removed from hot loops with a little work.

I think if the semantic on ExecState were to change such the setting an exception when another was already set was an error, then we could just remove these checks.  Assume that any sort of throwError function will set the exception (including proper line numbers, etc) ONLY if there isn't already an exception on the ExecState.  Then we just need to make sure that before leaving any node we should call handleExeception once to make sure the line numbers, etc. are updated if necessary.

We just need to be careful to educate the node tree to make sure continuing in an "exception was thrown, but I'm not yet ready to check it" state is OK.  So long as we record the proper line numbers, etc in the Error object, and make sure to update them before leaving any node... I think it should work.  Maybe I'll look at that tomorrow (when I'm more awake) if no one else has.

[Eric Seidel (no email)]

Ok, so I've further revised my theory for how we can remove all these if (exec-hadException()) checks.  Like before, we make exec->setException() ASSERT when already had an exception.  We remove *all* exception checks, except before any user-visible change.  User visible changes come in 3 flavors:
1.  Assignment nodes (we have to pretend execution stopped immediately after the exception, make sure it stops before we change any variable)
2.  Loops (hanging due to assignments never happening, thus loops never reaching their termination condition is a bad thing, loops need to check exceptions)
3.  non-source function calls (functions provided by the interpreter must not be called after an exception).

Otherwise execution can continue willie-nilly until we decide to stop at a convenient time.  SourceElementsNode (for example) should probably check exceptions in its loop just to make sure we don't continue on too far.

Sound totally crazy?  Or just crazy enough... ;)

[Darin Adler]

(In reply to comment #8)
> Ok, so I've further revised my theory for how we can remove all these if
> (exec-hadException()) checks.  Like before, we make exec->setException() ASSERT
> when already had an exception.  We remove *all* exception checks, except before
> any user-visible change.  User visible changes come in 3 flavors:
> 1.  Assignment nodes (we have to pretend execution stopped immediately after
> the exception, make sure it stops before we change any variable)
> 2.  Loops (hanging due to assignments never happening, thus loops never
> reaching their termination condition is a bad thing, loops need to check
> exceptions)
> 3.  non-source function calls (functions provided by the interpreter must not
> be called after an exception).

I think this sounds OK. But I believe "user-visible" is a pretty large category; get/set on an arbitrary object, toString on an arbitrary object, etc. I'm not sure how many of the checks you could remove with this trick.

[Eric Seidel (no email)]

I think another way to look at this is that we would need to check exceptions any time we're about to leave nodes.cpp (in a method that could have side effects).  Meaning before any "get" or "call" call.  As Maciej points out, it could be rather tricky to find all of these.  But I do believe that checking before get/call would result in fewer branches then the current situation of "check after any evaluate()" call.  It may not be worth bothering with this if bytecode is right around the corner...

[Oliver Hunt]

This bug is no longer valid -- squirrelfish has made it obsolete.