Closed Bug 1323096 Opened 3 years ago Closed 3 years ago

CacheIR: optimize more value[double] cases

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1322091 part 6 changed GuardIsInt32 to also check for doubles and convert these in-place to int32 if possible.

We use this for the typed array case, but it's now trivial to support value[int32-stored-as-double] for all of these stubs, we just need to check for this case when we attach the stub.

It would be nice to check this in tryAttachStub and then pass |int32_t index| to tryAttachDenseElement. Then we can also remove the indexVal_.isInt32() asserts.

Marking as P1: easy fix, improves IC coverage so it can be a big perf win in some cases, and we should be able to fix this before the end of the year.
Ion's typed/unboxed array stub also supports object[str] where str is an integer-string. We should probably support this in GuardIsInt32 as well, so we don't regress our Ion IC coverage. And (same story every time) then this will work for all stubs/JITs.
(In reply to Jan de Mooij [:jandem] from comment #1)
> We should probably support this in GuardIsInt32 as well, so
> we don't regress our Ion IC coverage.

After submitting this, I'm thinking we shouldn't do this in GuardIsInt32 but add a new op for that (or rename GuardIsInt32 to GuardIsInt32Key or so). The reason is that GuardIsInt32 may later be used for something else, like |int32 + int32|, and then the difference will be observable.
Attached patch PatchSplinter Review
With this patch, we are able to use all object[int32] stubs for object[double] (when the double is an integer) and object[string] (when the string is an int32 index).

The string case comes up with for-in in a few benchmarks. The micro-benchmark below is much faster now, especially with --no-ion (5 seconds => 1.5 seconds).

I didn't change the StringChar and MagicArguments stubs. Lazy arguments are only used if the index is int32, and StringChar is a little annoying because we have to restructure the code to first check everything and then emit the IR for the guards. It's not that difficult to fix, but not worth it now IMO.

function f(a) {
    var a = new Int32Array(100);
    var res;
    for (var i=0; i<100000; i++) {
        for (var j in a)
            res = a[j] + a[j] + a[j];
    }
    return res;
}
var t = new Date;
f();
print(new Date - t);
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8822069 - Flags: review?(evilpies)
Comment on attachment 8822069 [details] [diff] [review]
Patch

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

Bug 654190 would be cool to have for the string path. But still a nice improvement. It's so nice to have a central way of improving this.

::: js/src/jit/CacheIR.cpp
@@ +1298,5 @@
> +}
> +
> +bool
> +IRGenerator::maybeGuardInt32Index(Value index, ValOperandId indexId,
> +                                  int32_t* int32Index, Int32OperandId* int32IndexId)

It doesn't really fit the name, but using uint32_t would be more useful for the try functions.

::: js/src/jit/CacheIRCompiler.h
@@ +27,5 @@
>      _(GuardNoUnboxedExpando)              \
>      _(GuardAndLoadUnboxedExpando)         \
>      _(GuardNoDetachedTypedObjects)        \
>      _(GuardNoDenseElements)               \
> +    _(GetInt32IndexFromString)            \

GuardAndGet[Int32]IndexFromString, not sure if we want to drop the Int32, it's technically Int31 right now ;)
Attachment #8822069 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #4)
> It doesn't really fit the name, but using uint32_t would be more useful for
> the try functions.

Done.

> > +    _(GetInt32IndexFromString)            \
> 
> GuardAndGet[Int32]IndexFromString, not sure if we want to drop the Int32,
> it's technically Int31 right now ;)

Renamed it to GuardAndGetIndexFromString.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05a6f5bcb7a0
Optimize object[double] and object[string] like object[int32]. r=evilpie
https://hg.mozilla.org/mozilla-central/rev/05a6f5bcb7a0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.