1488775 - Add JIT support for string + number

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P2)

Description

[Ted Campbell [:tcampbell]]

We should have an IC to construct int+string ropes. This seems to come up when generating CSS property values.

> {
>   "name":"BinaryArith",
>   "file":"https://docs.google.com/static/document/client/js/2349105360-kix_main_i18n_integrated_kix_core.js",
>   "mode":0,
>   "line":1215,
>   "column":200,
>   "pc":"158db8d5245",
>   "op":"add",
>   "rhs":{
>     "type":"int32",
>     "value":1502212
>   },
>   "lhs":{
>     "type":"string",
>     "value":""
>   }
> },

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1488775 - Add String x Number concatenation IC r?tcampbell

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

A microbenchmark puts this at about 1/3 faster than the fallback path.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: string_number_microbenchmark.js

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Wrote an oomTest, but similar to Bug 1487760, wasn't able to actually demonstrate failure: 

    if (!('oomTest' in this))
        quit();
    
    // Add: String Number Concat OOM test
    var addOom = (a, b) => { return a + b; }
    
    function generate_digits(prefix, start) {
        digits = []
        number = ""+start+".25";
        for (var i = 1; i < 7; i++) {
            number = i + number;
            digits.push([prefix, Number(number), prefix + number]);
        }
        return digits;
    }
    
    // Trying to defeat dtoacache: Warm the IC with one set of digits, then actually oomTest
    // using another set.
    var warmup_digits = generate_digits("x", 1);
    var test_digits = generate_digits("x", 2);
    
    function ot(digits) {
        warmup(addOom, digits);
    }
    
    // Ensure ICs are warmed
    ot(warmup_digits);
    
    oomTest(() => { ot(test_digits); });

I am able to confirm we hit NumberToStringHelper (and therefore are being called from the IC), but never hit the OOM path.

Comment 5

[André Bargull [:anba]]

I found out it sometimes helps to add a loop into the oom-test function. So instead of just |ot(test_digits);|, maybe try |for (var i = 0; i < 20 /* or 50 or 100 */; ++i) { ot(test_digits); }| and see if it helps?

Comment 6

[Ted Campbell [:tcampbell]]

It turns out the case I was worried about only affects non-base-10 floating point. I believe the footgun remains for future consumers, but it is now unrelated to this patch.

Comment 7

[Ted Campbell [:tcampbell]]

Comment on attachment 9006706 [details]
Bug 1488775 - Add String x Number concatenation IC r?tcampbell

Ted Campbell [:tcampbell] has approved the revision.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9661ee2667d6
Add String x Number concatenation IC r=tcampbell

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

(Re-summaried to cover what actually landed)

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9661ee2667d6