Bug 185083 – Remove unneeded exception check from String.fromCharCode

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Remove unneeded exception check from String.fromCharCode

[Keith Miller]

Created attachment 339007 [details]
Patch

[Keith Miller]

Created attachment 339008 [details]
Patch

[Darin Adler]

Comment on attachment 339008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339008&action=review

> Source/JavaScriptCore/runtime/StringConstructor.cpp:80
>          scope.release();

Can we move the declarations of "vm" and "scope" after the if statement and remove this line of code too?

[Mark Lam]

Comment on attachment 339008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339008&action=review

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:80
>>          scope.release();
> 
> Can we move the declarations of "vm" and "scope" after the if statement and remove this line of code too?

Wouldn't the compiler know that vm and scope is not used until after this anyway?  scope.release() is a no-op on release builds.  I suspect that this move is unnecessary.

[Darin Adler]

Comment on attachment 339008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339008&action=review

>>> Source/JavaScriptCore/runtime/StringConstructor.cpp:80
>>>          scope.release();
>> 
>> Can we move the declarations of "vm" and "scope" after the if statement and remove this line of code too?
> 
> Wouldn't the compiler know that vm and scope is not used until after this anyway?  scope.release() is a no-op on release builds.  I suspect that this move is unnecessary.

Sure, it’s unnecessary to generate optimal code if it has no effect on the generated code.

But it’s also unnecessary to write the code the way it currently is; I think it’s clearer the way I suggest, and that’s how it was until a recent change, too. The fast case is clearly separated from the rest of the code with no exception handling code in it.

[Mark Lam]

Comment on attachment 339008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339008&action=review

>>>> Source/JavaScriptCore/runtime/StringConstructor.cpp:80
>>>>          scope.release();
>>> 
>>> Can we move the declarations of "vm" and "scope" after the if statement and remove this line of code too?
>> 
>> Wouldn't the compiler know that vm and scope is not used until after this anyway?  scope.release() is a no-op on release builds.  I suspect that this move is unnecessary.
> 
> Sure, it’s unnecessary to generate optimal code if it has no effect on the generated code.
> 
> But it’s also unnecessary to write the code the way it currently is; I think it’s clearer the way I suggest, and that’s how it was until a recent change, too. The fast case is clearly separated from the rest of the code with no exception handling code in it.

Actually, there's a need to declare the ThrowScope at the very top, and by convention we always do this where a ThrowScope is needed, unless there is a special reasons to make an exception from this rule (more on this below).

The reason for always declaring the ThrowScope at the top is because the purpose of a ThrowScope is to communicate to its caller that this function "may" throw an exception regardless which path is taken during the current invocation (fast, or slow).  This allows us to assert in the parent that a potential exception could have been thrown, and add an exception there if needed.  By declaring the ThrowScope later, we will cease to notify the caller that this function can potentially throw an exception, thereby defeating its purpose.

The only times we don't declare a ThrowScope at the top (and there should be very few of these) is if the function will actually eat / handle all exceptions that it encounters (i.e. it should have a CatchScope at the top), but in an embedded block, it needs a ThrowScope for various reasons.  There may be other reasons for not declaring the ThrowScope at the top. but this is the most obvious one I can think of at the moment.  In the case of stringFromCharCode(), I see no special reason why we should not declare ThrowScope at the top.

[Mark Lam]

Comment on attachment 339008 [details]
Patch

r=me

[Keith Miller]

Comment on attachment 339008 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339008&action=review

Thanks for the review!

>>>>> Source/JavaScriptCore/runtime/StringConstructor.cpp:80
>>>>>          scope.release();
>>>> 
>>>> Can we move the declarations of "vm" and "scope" after the if statement and remove this line of code too?
>>> 
>>> Wouldn't the compiler know that vm and scope is not used until after this anyway?  scope.release() is a no-op on release builds.  I suspect that this move is unnecessary.
>> 
>> Sure, it’s unnecessary to generate optimal code if it has no effect on the generated code.
>> 
>> But it’s also unnecessary to write the code the way it currently is; I think it’s clearer the way I suggest, and that’s how it was until a recent change, too. The fast case is clearly separated from the rest of the code with no exception handling code in it.
> 
> Actually, there's a need to declare the ThrowScope at the very top, and by convention we always do this where a ThrowScope is needed, unless there is a special reasons to make an exception from this rule (more on this below).
> 
> The reason for always declaring the ThrowScope at the top is because the purpose of a ThrowScope is to communicate to its caller that this function "may" throw an exception regardless which path is taken during the current invocation (fast, or slow).  This allows us to assert in the parent that a potential exception could have been thrown, and add an exception there if needed.  By declaring the ThrowScope later, we will cease to notify the caller that this function can potentially throw an exception, thereby defeating its purpose.
> 
> The only times we don't declare a ThrowScope at the top (and there should be very few of these) is if the function will actually eat / handle all exceptions that it encounters (i.e. it should have a CatchScope at the top), but in an embedded block, it needs a ThrowScope for various reasons.  There may be other reasons for not declaring the ThrowScope at the top. but this is the most obvious one I can think of at the moment.  In the case of stringFromCharCode(), I see no special reason why we should not declare ThrowScope at the top.

I agree with Mark here. Knowing which functions can throw exceptions is very useful. Given that the compiler will not emit any extra code in release there's really no reason to not have the ThrowScope at the top of the function.

[WebKit Commit Bot]

Comment on attachment 339008 [details]
Patch

Clearing flags on attachment: 339008

Committed r231171: <https://trac.webkit.org/changeset/231171>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39849327>