Closed Bug 1337763 Opened 3 years ago Closed 3 years ago

Attach In stub for DenseElementHole

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: evilpie, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

The most common failure to attach |in| on twitter and gdocs is related to having an int32 index. I am relatively certain that supporting DenseElementHole for |in| would solve those. And it kind of makes sense in general, because why would use use |in|? Because you except to have holes. This is what happens for various self-hosted functions that use |in| like ArrayMap etc.
This is probably my regression in Bug 1337811
Assignee: nobody → tcampbell
Comment on attachment 8840601 [details]
Bug 1337763 - Factor out GeneratePrototypeHoleGuards

https://reviewboard.mozilla.org/r/115066/#review117076

LGTM.
Attachment #8840601 - Flags: review?(jdemooij) → review+
Comment on attachment 8840602 [details]
Bug 1337763 - Add DenseInHole IC to CacheIR

https://reviewboard.mozilla.org/r/115068/#review117078

Nice!

::: js/src/jit/CacheIR.h:212
(Diff revision 1)
>      _(LoadUnboxedPropertyResult)          \
>      _(LoadTypedObjectResult)              \
>      _(LoadDenseElementResult)             \
>      _(LoadDenseElementHoleResult)         \
>      _(LoadDenseElementExistsResult)       \
> +    _(LoadDenseElementIsHoleResult)       \

Nit: IsHole suggests we only handle holes or return true for holes and false if not a hole. I think it'd make sense to call it LoadDenseElementExistsHoleResult, for symmetry with LoadDenseElement{Hole}Result.

::: js/src/jit/CacheIR.cpp:1792
(Diff revision 1)
> +        return false;
> +
> +    // Guard on the shape, to prevent non-dense elements from appearing.
> +    writer.guardShape(objId, obj->as<NativeObject>().lastProperty());
> +
> +    GeneratePrototypeHoleGuards(writer, obj, objId);

Please make sure we have tests for this, for instance comment out this line and see if you get any jit-test failures.
Attachment #8840602 - Flags: review?(jdemooij) → review+
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b0b679a65a2
Factor out GeneratePrototypeHoleGuards r=jandem
https://hg.mozilla.org/integration/autoland/rev/8c19e05bea72
Add DenseInHole IC to CacheIR r=jandem
(The ion/testInArray tests do cover prototype modification cases)
https://hg.mozilla.org/mozilla-central/rev/9b0b679a65a2
https://hg.mozilla.org/mozilla-central/rev/8c19e05bea72
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.