1323037 - CacheIR: support dense elements with holes

Status: RESOLVED FIXED

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

Description

[Tom S [:evilpie]]

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.

Comment 1

[Tom S [:evilpie]]

Attachment: CacheIR for dense elements with holes

I think this is basically ready minus one open question about guarding on the proto.

Comment 2

[Jan de Mooij [:jandem]]

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:

http://searchfox.org/mozilla-central/rev/5ee2bd8800b007d6c120d9521d5bf01131885afb/js/src/jsobj.h#257-261

Comment 3

[Tom S [:evilpie]]

Attachment: v2 - CacheIR for dense elements with holes

Thanks Jan. Will be interesting to see if this gives us a win anywhere.

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Jan de Mooij [:jandem]]

(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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b114f7412f
CacheIR for dense elements with holes. r=jandem

Comment 7

[Tom S [:evilpie]]

Attachment: Benchmark

I used this for testing. With --no-ion, before 349ms after 68ms.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a1b114f7412f