Bug 16471 – Completions need to be smaller (or not exist at all)

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

[Eric Seidel (no email)]

Completions need to be smaller (or not exist at all)

Looking at the Shark samples, it seems to me we're spending a significant amount of time copying completions around (during the return from an execute() call).  Perhaps I'm reading the asm wrong, and it's simply virtual function call overhead, but I think if we make Completions smaller (i.e. remove the m_target member, and possibly even the type -- thus just turning all executes into evaluates and storing the type and target on the ExecState), we'd see a win.

[Eric Seidel (no email)]

Yeah, looking again at the asm, I'm quite certain we're spending tons of extra time copying these return values onto the stack after each function call.  Most of the time all we care about is the value.  Storing the completion type and target on the ExecState and just returning a value could be a large win.

[Darin Adler]

Created attachment 18003 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 18003 [details]
patch

Ok, first of all, this is an absolutely wonderful change!  2.4%, wow.

I have two nits:

First,
CaseClauseNode::executeStatements:
seems strange that that returns jsUndefined() instead of 0 (like other excute statements).  I didn't follow the logic all the way through to see if jsUndefined was necessary in this case.


Second,
I dislike exposing of setCompletionType.  I think that doing so is error prone, and leads to less-clean code.

I like your use of setErrorCompletion and setBreakCompletion and would suggest changing the expected for *all* completion returns to:

return setNormalCompletion(ret);
return setNormalCompletion();
return setThrowCompletion(val);
return setInteruptedCompletion();

That would basically make setCompletionType an "implementation detail" and in all cases (except perhaps due to try/catch/finally) could be a private method on ExecState.  It would also turn current 2-line statements of:
setInteruptedCompletion()
return 0;

into a single line:
return setInterupedCompletion();


Oh, and also:
I kinda laughed at "continueDoLoop", I probably would have named it doWhileLoop. :)

Oh, and if you change to use the "return setNormalCompletion();" idiom, you would end up make hitStatement() not alter the completion type directly.  (which looks kinda odd to me currently).

Great patch.  Landable as is.  I'd prefer to see it change to the return set*Completion(); idiom.

[Darin Adler]

(In reply to comment #3)
> CaseClauseNode::executeStatements:
> seems strange that that returns jsUndefined() instead of 0 (like other excute
> statements).  I didn't follow the logic all the way through to see if
> jsUndefined was necessary in this case.

I noticed and preserved this strangeness because it was not relevant to performance and I didn't feel I should make a correctness fix without a test case.

> I dislike exposing of setCompletionType.  I think that doing so is error prone,
> and leads to less-clean code.

I share your bias. An earlier version of the patch had separate calls for separate completions, although they didn't have the return value trick.

I added setCompletionType because of TryNode::execute, and I won't be able to remove it because of that function.

What I didn't think of was including the JSValue pointers to avoid the multiple line return statements. I think that's a good idea.

> Oh, and if you change to use the "return setNormalCompletion();" idiom, you
> would end up make hitStatement() not alter the completion type directly. 
> (which looks kinda odd to me currently).

No, I won't change that; it would make the code bigger for debugging -- it's a feature that the code to set the completion type for the debugger case is not compiled in-line.

[Darin Adler]

r28907