Closed Bug 1323037 Opened 3 years ago Closed 3 years ago

CacheIR: support dense elements with holes


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




Tracking Status
firefox53 --- fixed


(Reporter: evilpie, Assigned: evilpie)


(Blocks 1 open bug)



(2 files, 1 obsolete file)

Ion has support for loading undefined when the index is a hole, we should support the same in CacheIR. So we get this support in Baseline right now and Ion is going to keep supporting it.
I think this is basically ready minus one open question about guarding on the proto.
Comment on attachment 8818063 [details] [diff] [review]
CacheIR for dense elements with holes

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

Thanks for doing this! Looks great.

::: js/src/jit/CacheIR.cpp
@@ +911,4 @@
>  }
>  bool
> +CanAttachDenseElementHole(JSObject* obj)

Nit: static bool

@@ +953,5 @@
> +    if (!obj->isNative() || !CanAttachDenseElementHole(obj))
> +        return false;
> +
> +    // Guard on the shape and group, to prevent non-dense elements from appearing.
> +    // XXX there was no group guard in ion?!

We don't need a group guard. The code is protecting against obj->isIndexed() becoming true, and that is covered by the shape guard - we will generate a new shape when we set that flag.

(FWIW, the "to prevent non-dense elements from appearing" part is incomplete, we also guard on the shape to ensure the object is native, does not have a weird clasp etc. Basically everything in CanAttachDenseElementHole. Maybe we can improve the comment a bit.)

@@ +965,5 @@
> +    JSObject* pobj = obj->staticPrototype();
> +    while (pobj) {
> +        ObjOperandId protoId = writer.loadObject(pobj);
> +
> +        // XXX Ion only guards in the group case, but I copied this from GeneratePrototypeGuards

I think we can follow the Ion code. If we change a singleton's proto, we trigger a shape change and the code below guards on the shape. See also this comment:
Thanks Jan. Will be interesting to see if this gives us a win anywhere.
Attachment #8818063 - Attachment is obsolete: true
Attachment #8818240 - Flags: review?(jdemooij)
Attachment #8818240 - Attachment is patch: true
Comment on attachment 8818240 [details] [diff] [review]
v2 - CacheIR for dense elements with holes

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

Nice to have Baseline ICs catching up with Ion.

You probably did this already but so far for every conversion I've done some quick micro-benchmarking or stepping through the generated code to make sure it works.

::: js/src/jit/BaselineCacheIR.cpp
@@ +1723,5 @@
> +
> +    // Load obj->elements.
> +    masm.loadPtr(Address(obj, NativeObject::offsetOfElements()), scratch);
> +
> +    // Makue sure there are no dense elements.

Nit: Make

::: js/src/jit/CacheIR.cpp
@@ +940,5 @@
> +        if (proto->as<NativeObject>().getDenseInitializedLength() != 0)
> +            return false;
> +
> +        obj = proto;
> +    } while (obj);

Nit: I think proto here is always non-null (because we break out of the loop if non-null), so we could turn this into a while (true) {} loop.
Attachment #8818240 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #4)
> (because we break out of the loop if non-null)

s/non-null/null/ obviously.
Pushed by
CacheIR for dense elements with holes. r=jandem
Attached file Benchmark
I used this for testing. With --no-ion, before 349ms after 68ms.
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.