1323190 - Attach Proxy GetElem IC

Status: RESOLVED FIXED

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

Description

[Tom S. (please needinfo tschuster)]

It's not uncommon to see a Proxy with GetElem and some integer index. It almost looks like tryAttachProxy could already work with indexes.

Comment 1

[Jan de Mooij [:jandem]]

Good idea. I agree we could probably fix this with only CacheIR.cpp/h changes.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: Attach Proxy GetElem IC

This is so simple there is no good reason not to do. I see a performance win locally with ES6 Proxy, but the testcase in bug 917509 doesn't improve at all.

Comment 3

[Tom S. (please needinfo tschuster)]

The performance of this is much improved by inlining the Proxy::get call into ProxyGetPropertyByValue. I just have to find a way of doing this that isn't totally ugly.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8822746 [details] [diff] [review]
Attach Proxy GetElem IC

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

Nice, r=me with comment below addressed.

::: js/src/jit/CacheIR.cpp
@@ +143,5 @@
>                  return true;
>          }
>  
> +        if (tryAttachProxyElement(obj, objId))
> +            return true;

When we get here, maybeGuardInt32Index may have inserted guards for the index (but not if the index is a non-integer). Shouldn't we move this call before maybeGuardInt32Index, so we don't emit these unnecessary guards?

(I should have added a "return false;" at the end of the "if (maybeGuardInt32Index(" block to make this more obvious..)

Comment 5

[Tom S. (please needinfo tschuster)]

Good catch, this actually was causing issues on try, because on x86 we were running out of registers.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #5)
> Good catch, this actually was causing issues on try, because on x86 we were
> running out of registers.

Hm we shouldn't be running out of registers because of this - the number of registers we can use for an instruction is always the same because we can spill registers the instruction doesn't use. I'll investigate.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> Hm we shouldn't be running out of registers because of this - the number of
> registers we can use for an instruction is always the same because we can
> spill registers the instruction doesn't use. I'll investigate.

Filed bug 1328227.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68f5a2cc409
Attach Proxy GetElem IC. r=jandem

Comment 10

[Tom S. (please needinfo tschuster)]

I still want to optimize the C++ side a bit more in this bug. Note: This doesn't handle just int32, but every kind of value type. (We already supported string/symbol before)

Comment 11

[Tom S. (please needinfo tschuster)]

Attachment: Remove Proxy friendapi

Because we moved ObjectOps to a different struct we now define that in the same file and can stop exporting those in the friendapi. Only WeakmapKeyDelegate is still used directly in the header. proxy_Call and proxy_Construct need to accessible in some other files. I also left stuff like proxy_DeleteProperty, where we don't directly call the Proxy:: implementation.

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8823347 [details] [diff] [review]
Remove Proxy friendapi

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

Nice simplification :)

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67504d1d4593
Remove Proxy friendapi. r=arai

Comment 15

[Boris Zbarsky [:bzbarsky]]

Is there a followup bug for the C++ changes mentioned above?

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67504d1d4593

Comment 17

[Tom S. (please needinfo tschuster)]

Attachment: Inline Proxy::get

Inlining Proxy::get into the caller functions gives measurable performance improvements. For example I extracted the "mootools - *" tests from bug 875815 and we go from ~2100ms to ~1900ms.

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8824039 [details] [diff] [review]
Inline Proxy::get

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

Looks good.

::: js/src/proxy/Proxy.cpp
@@ +6,4 @@
>  
>  #include "js/Proxy.h"
>  
> +#include "mozilla/Attributes.h"

This should go before the js/Proxy.h #include I think.

Comment 19

[Tom S. (please needinfo tschuster)]

Changing the include order makes it check-spidermonkey style complain. I think the first include should be proxy/Proxy.h anyway?

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96534237bb1e
Inline Proxy::get into JIT VM functions. r=jandem

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96534237bb1e