Jan de Mooij [:jandem] Assignee
	Description • 9 years ago

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.

	Brian Hackett [Laid off!]
	Comment 1 • 9 years ago

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.

	Jan de Mooij [:jandem] Assignee
	Comment 2 • 9 years ago

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

	Pulsebot
	Comment 3 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/14be11ac3fd7

	Brian Hackett [Laid off!]
	Comment 4 • 9 years ago

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

	Phil Ringnalda (:philor)
	Comment 5 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/14be11ac3fd7