CacheIR: optimize more value[double] cases

RESOLVED FIXED in Firefox 53

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
(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.
(Assignee)

Comment 3

2 years ago
Created attachment 8822069 [details] [diff] [review]
Patch

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+
(Assignee)

Comment 5

2 years ago
(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.

Comment 6

2 years ago
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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/05a6f5bcb7a0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.