Optimize GETELEM with constant string-or-symbol index

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

Posted patch PatchSplinter Review
With this patch we use the getPropTryConstant path for GETELEM with constant string-or-symbol index. This matters mostly for symbols because getting Symbol.iterator for instance is always compiled as a GETELEM.

This improves some micro-benchmark based on the destructuring test in bug 1177319 from 1474 to 640 ms.
Attachment #8660056 - Flags: review?(bhackett1024)
Comment on attachment 8660056 [details] [diff] [review]
Patch

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

::: js/src/jit/IonBuilder.cpp
@@ +8419,5 @@
> +    if (!ValueToIdPure(index->constantValue(), &id))
> +        return true;
> +
> +    if (!JSID_IS_STRING(id) && !JSID_IS_SYMBOL(id))
> +        return true;

I don't think these tests are sufficient.  If the id is a numeral then id != IdToTypeId(id) and we can eventually bust an assert under ObjectGroup::maybeGetProperty.

I think that returning true here if id != IdToTypeId(id) is the best option here for now.  It would be nicer if IdToTypeId didn't exist and we just flattened together all non-PropertyName/Symbol ids into JSID_VOID in type information, but things aren't there yet.
Attachment #8660056 - Flags: review?(bhackett1024) → review+
(In reply to Brian Hackett (:bhackett) from comment #1)
> I don't think these tests are sufficient.  If the id is a numeral then id !=
> IdToTypeId(id) and we can eventually bust an assert under
> ObjectGroup::maybeGetProperty.
> 
> I think that returning true here if id != IdToTypeId(id) is the best option
> here for now.

id != IdToTypeId can only ever be true if JSID_IS_INT(id), and that case is handled fine by:

    if (!JSID_IS_STRING(id) && !JSID_IS_SYMBOL(id))
        return true;

I can change it to what you suggested though, it does seem a bit nicer.
(In reply to Jan de Mooij [:jandem] from comment #2)
> (In reply to Brian Hackett (:bhackett) from comment #1)
> > I don't think these tests are sufficient.  If the id is a numeral then id !=
> > IdToTypeId(id) and we can eventually bust an assert under
> > ObjectGroup::maybeGetProperty.
> > 
> > I think that returning true here if id != IdToTypeId(id) is the best option
> > here for now.
> 
> id != IdToTypeId can only ever be true if JSID_IS_INT(id), and that case is
> handled fine by:
> 
>     if (!JSID_IS_STRING(id) && !JSID_IS_SYMBOL(id))
>         return true;
> 
> I can change it to what you suggested though, it does seem a bit nicer.

Oh, I'm sorry, I forgot about bug 1052491 and was remembering the old semantics for IdToTypeId where it converted numeric atom ids to JSID_VOID as well.
https://hg.mozilla.org/mozilla-central/rev/14be11ac3fd7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.