Closed Bug 1198861 Opened 5 years ago Closed 5 years ago

Improve aliasing information and type barrier handling for unboxed arrays


(Core :: JavaScript Engine: JIT, defect)

Not set



Tracking Status
firefox43 --- fixed


(Reporter: bhackett1024, Assigned: bhackett1024)




(2 files)

Attached patch patchSplinter Review
The attached patch fixes a couple performance faults with unboxed arrays on sunspider-crypto-aes:

- Improves handling of aliased loads and stores to match that used for normal arrays.
- Avoids type barriers in cases where the objects being filtered out are similar to those being accepted.  This affects both normal and unboxed arrays but we pay a higher price for unboxed arrays because type barrier code generation in this case is so bad --- we load the unboxed pointer, box it, then unbox it, then check its group.  That needs to get fixed too but is more involved, and when possible it's best if we can just avoid the barrier entirely.
Attachment #8652982 - Flags: review?(jdemooij)
Comment on attachment 8652982 [details] [diff] [review]

Review of attachment 8652982 [details] [diff] [review]:

Sorry for the delay.

::: js/src/jit/MIR.cpp
@@ +5101,5 @@
> +                TypeSet::ObjectKey* key = property.maybeTypes()->getObject(i);
> +                if (!key)
> +                    continue;
> +
> +                if (!observed->unknownObject()) {

Could invert this condition and add a continue to get rid of one level of indentation.
Attachment #8652982 - Flags: review?(jdemooij) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
The last patch didn't really help crypto-aes on ss --- this and the other ss tests are so short running that profiling them is difficult.  I think the new type barrier logic actually made things worse; Ion code perf doesn't matter much in this test (since it's so short) and we ended up doing an additional compilation due to these heuristics.  So this patch removes them, and improves handling of type barriers when reading unboxed ObjectOrNull pointers.  That doesn't help perf on this test either, of course.  This patch also fixes some issues with baseline compilation where we were running out of stubs at some access sites when unboxed arrays were being used.
Attachment #8657316 - Flags: review?(jdemooij)
Comment on attachment 8657316 [details] [diff] [review]
more random things

Review of attachment 8657316 [details] [diff] [review]:

::: js/src/jit/BaselineIC.cpp
@@ +3980,5 @@
>          if (!proto->isNative())
>              return false;
>          if (proto->as<NativeObject>().lastProperty() != nstub->shape(i + 1))
>              return false;
> +        proto = proto->getProto();

Ugh good catch.

@@ +4026,5 @@
> +        if (!iter->isSetElem_DenseOrUnboxedArrayAdd())
> +            continue;
> +
> +        ICSetElem_DenseOrUnboxedArrayAdd* nstub = iter->toSetElem_DenseOrUnboxedArrayAdd();
> +        if (obj->getGroup(cx) != nstub->group())

getGroup is fallible, can it leave a pending exception on the cx?
Attachment #8657316 - Flags: review?(jdemooij) → review+
This patch is a huge regression on Octane.
Flags: needinfo?(bhackett1024)
Type set guards with the new BarrierKind were always bailing out on primitive values.  I fixed this and added some debugging code to make sure the barrier kind is acting correctly.
Flags: needinfo?(bhackett1024)
Seems like a few benchmarks on awfy aren't too happy with this push:
Well, octane looked fine on my machine.  I don't think I'm going to try to reland this, the ss tests that improved on my machine didn't improve on awfy.

Backed out:
You need to log in before you can comment on or make changes to this bug.