Tom S [:evilpie] Assignee
	Description • 8 years ago

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

	Jan de Mooij [:jandem]
	Comment 1 • 8 years ago

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

	Tom S [:evilpie] Assignee
	Comment 2 • 8 years ago

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.

	Tom S [:evilpie] Assignee
	Comment 3 • 8 years ago

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.

	Jan de Mooij [:jandem]
	Comment 4 • 8 years ago

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

	Tom S [:evilpie] Assignee
	Comment 5 • 8 years ago

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

	Jan de Mooij [:jandem]
	Comment 6 • 8 years ago

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

	Jan de Mooij [:jandem]
	Comment 7 • 8 years ago

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

	Pulsebot
	Comment 8 • 8 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68f5a2cc409
Attach Proxy GetElem IC. r=jandem

	Tom S [:evilpie] Assignee
	Comment 10 • 8 years ago

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)

	Tom S [:evilpie] Assignee
	Comment 11 • 8 years ago

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.

	Tooru Fujisawa [:arai]
	Comment 12 • 8 years ago

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

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

Nice simplification :)

	Phil Ringnalda (:philor)
	Comment 13 • 8 years ago
bugherder

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

	Pulsebot
	Comment 14 • 8 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67504d1d4593
Remove Proxy friendapi. r=arai

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 16 • 8 years ago
bugherder

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

	Tom S [:evilpie] Assignee
	Comment 17 • 8 years ago

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.

	Jan de Mooij [:jandem]
	Comment 18 • 8 years ago

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.

	Tom S [:evilpie] Assignee
	Comment 19 • 8 years ago

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

	Pulsebot
	Comment 20 • 8 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96534237bb1e
Inline Proxy::get into JIT VM functions. r=jandem

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 8 years ago
bugherder

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