1142814 - Optimize String.fromCharCode() when the number of args is small but > 1

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Shumway calls String.fromCharCode() with two arguments frequently, and this
does an unnecessary malloc that can be avoide easily..

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Optimize String.fromCharCode() when the number of args is small but > 1

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8577005 [details] [diff] [review]
Optimize String.fromCharCode() when the number of args is small but > 1

Review of attachment 8577005 [details] [diff] [review]:
-----------------------------------------------------------------

Avoiding malloc is always nice.

::: js/src/jsstr.cpp
@@ +4031,5 @@
> +    for (unsigned i = 0; i < args.length(); i++) {
> +        uint16_t code;
> +        if (!ToUint16(cx, args[i], &code)) {
> +            return false;
> +        }

Nit: no {}

@@ +4037,5 @@
> +    }
> +    JSString *str = NewStringCopyN<CanGC>(cx, chars, args.length());
> +    if (!str) {
> +        return false;
> +    }

And here.

Comment 3

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2693283b5d8b

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/df62dfbaa245 for apparently breaking bc1:

https://treeherder.mozilla.org/logviewer.html#?job_id=7684028&repo=mozilla-inbound

Comment 5

[Nicholas Nethercote [inactive]]

Somehow I accidentally incorporated some unrelated and broken changes to JSDOMParser.js in my first push. Relanding without those changes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/24640e5e0d3d

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24640e5e0d3d

Comment 7

[Gary [:streetwolf52]]

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1144081 for a regression problem that might be caused by this patch.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Gary [:streetwolf] from comment #7)
> Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1144081 for a
> regression problem that might be caused by this patch.

(Commented there. Other bugs in that range seem more likely than this one.)