Closed Bug 1488775 Opened Last year Closed Last year

Add JIT support for string + number

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: tcampbell, Assigned: mgaudet)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p1:f64])

Attachments

(2 files)

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":""
>   }
> },
Assignee: nobody → mgaudet
Status: NEW → ASSIGNED
A microbenchmark puts this at about 1/3 faster than the fallback path.
See Also: → 1359751
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.
See Also: → 1487760
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?
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.
Depends on: 1489492
Whiteboard: [qf:p1:f64]
Attachment #9006706 - Attachment description: Bug 1488775 - Add String x Number concatenation IC r=tcampbell → Bug 1488775 - Add String x Number concatenation IC r?tcampbell
Comment on attachment 9006706 [details]
Bug 1488775 - Add String x Number concatenation IC r?tcampbell

Ted Campbell [:tcampbell] has approved the revision.
Attachment #9006706 - Flags: review+
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9661ee2667d6
Add String x Number concatenation IC r=tcampbell
(Re-summaried to cover what actually landed)
Summary: Add JIT support for string + integer → Add JIT support for string + number
https://hg.mozilla.org/mozilla-central/rev/9661ee2667d6
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.