Bug 167454 – [JSC] Optimize Number.prototype.toString on Int32 / Int52 / Double

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

[Yusuke Suzuki]

[JSC] Optimize Number.prototype.toString on Int32 / Int52 / Double

[Yusuke Suzuki]

Created attachment 299804 [details]
Patch

[Saam Barati]

Comment on attachment 299804 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGOperations.h:162
> +char* JIT_OPERATION operationInt32ToString(ExecState*, int32_t, int32_t);

Why not make these return JSCell*?

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:407
> +                            result = String::numberToStringECMAScript(value.asNumber());

Why not always call this?

> Source/JavaScriptCore/runtime/StringConstructor.cpp:47
> +  fromCharCode          stringFromCharCode         DontEnum|Function 1 FromCharCodeIntrinsic

I think this should be a separate patch.

[Saam Barati]

(In reply to comment #2)
> Comment on attachment 299804 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299804&action=review
> 
> > Source/JavaScriptCore/dfg/DFGOperations.h:162
> > +char* JIT_OPERATION operationInt32ToString(ExecState*, int32_t, int32_t);
> 
> Why not make these return JSCell*?
> 
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:407
> > +                            result = String::numberToStringECMAScript(value.asNumber());
> 
> Why not always call this?
> 
> > Source/JavaScriptCore/runtime/StringConstructor.cpp:47
> > +  fromCharCode          stringFromCharCode         DontEnum|Function 1 FromCharCodeIntrinsic
> 
> I think this should be a separate patch.
Also please fix 32-bit and windows build issues.

[Yusuke Suzuki]

Comment on attachment 299804 [details]
Patch

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

Thanks!

>>> Source/JavaScriptCore/dfg/DFGOperations.h:162
>>> +char* JIT_OPERATION operationInt32ToString(ExecState*, int32_t, int32_t);
>> 
>> Why not make these return JSCell*?
> 
> Also please fix 32-bit and windows build issues.

This is because the signatures of these functions are typedefed as P_ (returning pointer) to reuse the existing implementation of callOperation.
I'll check Win32 build issue.

>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:407
>> +                            result = String::numberToStringECMAScript(value.asNumber());
> 
> Why not always call this?

I followed the style of the ValueAdd case. But maybe, calling numberToStringECMAScript is OK.
I'll check that.

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:47
>> +  fromCharCode          stringFromCharCode         DontEnum|Function 1 FromCharCodeIntrinsic
> 
> I think this should be a separate patch.

I've just uploaded this part in the separate patch :)
https://bugs.webkit.org/show_bug.cgi?id=167505

[Saam Barati]

(In reply to comment #4)
> Comment on attachment 299804 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299804&action=review
> 
> Thanks!
> 
> >>> Source/JavaScriptCore/dfg/DFGOperations.h:162
> >>> +char* JIT_OPERATION operationInt32ToString(ExecState*, int32_t, int32_t);
> >> 
> >> Why not make these return JSCell*?
> > 
> > Also please fix 32-bit and windows build issues.
> 
> This is because the signatures of these functions are typedefed as P_
> (returning pointer) to reuse the existing implementation of callOperation.
> I'll check Win32 build issue.
> 
> >> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:407
> >> +                            result = String::numberToStringECMAScript(value.asNumber());
> > 
> > Why not always call this?
> 
> I followed the style of the ValueAdd case. But maybe, calling
> numberToStringECMAScript is OK.
> I'll check that.
I think it's OK to be consistent with what Add does here.
> 
> >> Source/JavaScriptCore/runtime/StringConstructor.cpp:47
> >> +  fromCharCode          stringFromCharCode         DontEnum|Function 1 FromCharCodeIntrinsic
> > 
> > I think this should be a separate patch.
> 
> I've just uploaded this part in the separate patch :)
> https://bugs.webkit.org/show_bug.cgi?id=167505

[Yusuke Suzuki]

Oops, I forgot this patch! I'll land it with pointed fixes.

[Yusuke Suzuki]

Created attachment 304995 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Committed r214219: <http://trac.webkit.org/changeset/214219>