Don't attach a GetDenseElement stub if the object has no dense elements

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

I noticed this in bug 1306450. It's silly: we were doing a GETELEM on a typed array, but we first attached a dense stub for no good reason (typed arrays can't have dense elements).
Attachment #8796562 - Flags: review?(hv1989)
Comment on attachment 8796562 [details] [diff] [review]
Patch

Review of attachment 8796562 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch!

::: js/src/jit/IonCaches.cpp
@@ +3827,5 @@
>      if (!obj->isNative() || !idval.isInt32())
>          return true;
>  
> +    if (uint32_t(idval.toInt32()) >= obj->as<NativeObject>().getDenseInitializedLength())
> +        return true;

Shouldn't we split this into more verbose parts? I know it is the same, but it is easier to see all reasons, instead of just hiding it in one line?

if (idval.toInt32() < 0)
    return true;

if (obj->as<NativeObject>().getDenseInitializedLength() == 0)
    return true;

if (idval.toInt32() >= obj->as<NativeObject>().getDenseInitializedLength())
    return true;
Attachment #8796562 - Flags: review?(hv1989) → review+
(Assignee)

Comment 2

2 years ago
(In reply to Hannes Verschore [:h4writer] from comment #1)
> Shouldn't we split this into more verbose parts? I know it is the same, but
> it is easier to see all reasons, instead of just hiding it in one line?

Well it's just a bounds check, I think we have many places where we do a single compare like this one.
(In reply to Jan de Mooij [:jandem] from comment #2)
> (In reply to Hannes Verschore [:h4writer] from comment #1)
> > Shouldn't we split this into more verbose parts? I know it is the same, but
> > it is easier to see all reasons, instead of just hiding it in one line?
> 
> Well it's just a bounds check, I think we have many places where we do a
> single compare like this one.

Okay, r+ either way.
Priority: -- → P1

Comment 4

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7cb15d415ee
Don't attach an Ion GetDenseElement stub if we're not accessing a dense element. r=h4writer

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7cb15d415ee
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1404636
(Assignee)

Updated

10 months ago
No longer depends on: 1404636
You need to log in before you can comment on or make changes to this bug.