Bug 205583 – REGRESSION (r253867): Six test262 tests broken

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

[Alexey Proskuryakov]

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20Test262%20%28Tests%29/builds/8064/steps/test262-test/logs/stdio

---------------NEW FAILING TESTS SUMMARY---------------

FAIL test/intl402/Collator/prototype/compare/compare-function-name.js (default)
Full Output:
Exception: Test262Error: descriptor value should be 

FAIL test/intl402/Collator/prototype/compare/compare-function-name.js (strict mode)
Full Output:
Exception: Test262Error: descriptor value should be 

FAIL test/intl402/DateTimeFormat/prototype/format/format-function-name.js (default)
Full Output:
Exception: Test262Error: descriptor value should be 

FAIL test/intl402/DateTimeFormat/prototype/format/format-function-name.js (strict mode)
Full Output:
Exception: Test262Error: descriptor value should be 

FAIL test/intl402/NumberFormat/prototype/format/format-function-name.js (default)
Full Output:
Exception: Test262Error: descriptor value should be 

FAIL test/intl402/NumberFormat/prototype/format/format-function-name.js (strict mode)
Full Output:
Exception: Test262Error: descriptor value should be 

---------------------------------------------------------

[Radar WebKit Bug Importer]

<rdar://problem/58183421>

[Yusuke Suzuki]

Created attachment 386404 [details]
Patch

[Darin Adler]

Comment on attachment 386404 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSFunction.cpp:893
>              string = jsString(globalObject, jsString(vm, "bound "_s), name);

Seems unnecessarily inefficient to call jsString(vm, "bound "_s) because jsString is already overridden so it can take a WTF::String. So it can just be "bound "_s. That will save allocating and garbage collecting a JSString cell every time this is called. Also, we could optimize this case even further by overloading jsString for ASCIILiteral too, if we need to.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #3)
> Comment on attachment 386404 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386404&action=review
> 
> > Source/JavaScriptCore/runtime/JSFunction.cpp:893
> >              string = jsString(globalObject, jsString(vm, "bound "_s), name);
> 
> Seems unnecessarily inefficient to call jsString(vm, "bound "_s) because
> jsString is already overridden so it can take a WTF::String. So it can just
> be "bound "_s. That will save allocating and garbage collecting a JSString
> cell every time this is called. Also, we could optimize this case even
> further by overloading jsString for ASCIILiteral too, if we need to.

Right, I'll put bound string in SmallStrings to avoid this allocation.

[Yusuke Suzuki]

Created attachment 386621 [details]
Patch

[Mark Lam]

Comment on attachment 386621 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:12
> +        If a function has empty name, a bound function should have "bound " name.
> +        But Intl prototypes' bound functions are exceptions: these JSBoundFunctions have empty name.
> +        In this patch, we pass `nullptr` for the JSBoundFunction::create's nameMayBeNull parameter of Intl prototypes' bound functions,
> +        to generate empty string name for these bound functions instead of "bound ".
> +        This fixes test262 failures.

nit: Would be nice to quote the urls to the relevant spec here if you have them.

[Yusuke Suzuki]

Comment on attachment 386621 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:12
>> +        This fixes test262 failures.
> 
> nit: Would be nice to quote the urls to the relevant spec here if you have them.

Thanks, fixed.

> Source/JavaScriptCore/runtime/SmallStrings.cpp:64
> +    initialize(&vm, m_boundPrefixString, "bound ");

Added `visitChildren` for this.

[Yusuke Suzuki]

Committed r253982: <https://trac.webkit.org/changeset/253982>