Katelyn Gadd (:kael) Reporter
	Description • 12 years ago

At present, if you create a property (using Object.defineProperty) that has a getter/setter, accesses to that property are tremendously slow - 10-100x slower than calling the getter/setter function directly, if not worse.

Having some amount of performance penalty for the indirection is reasonable, but at present this means that getters are basically unusable in code that needs to be performant.

In particular, it's important for getters/setters to be performant because they allow writing better code. For example:

foo.Property++
instead of
foo.set_Property(foo.get_Property() + 1);

getters/setters are also of value for lazy evaluation of expressions because they provide a simple way to expose lazy evaluation without changing the interface of a type. The alternative (using a function) means that now the lazily evaluated expression must always be a function call, even if it doesn't always require lazy evaluation and can be a constant.

	Luke Wagner [:luke]
	Comment 1 • 12 years ago

I've also seen heavy getter usage in other hand-written JS game frameworks.  It is even reasonable to assume that they are fast since the identity of the callee is determined by the shape which is actually better than a normal callprop (at least, w/o branding).

	David Mandelin [:dmandelin]
	Comment 2 • 12 years ago

Some data:

I just took another look at the JSPerf benchmark. The archived results show Chrome 21 (18kops/s, 2x slower than vanilla property), Chrome 22 (28kops/s, 3x slower than vanilla), and Firefox 18 (21kops/s, 2x slower than vanilla) doing the best at getters and setters.

(I note that Firefox 18 shows a lower rate overall than Fx 16, which may be something to look into, although Fx18 is only one sample.)

I tested with IM and got 20kops/s, which is 2x slower than vanilla. 

Remarks/Questions:

Kevin, what do you think browsers, and Firefox in particular, should aim for with getters/setters? All other browsers are very slow at these tests, and probably will be so for some time. Would you just go ahead and target Chrome and Firefox more if they were the only browsers good at this, or would you avoid it in order to work in more browsers?

The second question, for the JIT people, is how fast can we hope to make this? In this case (which is probably about the usual), the property is defined in the prototype and uses a getter, which is a function that just returns a property. With perfect static information, it seems we could reduce this to the same thing as a property get. Can we get perfect static information? If not, how many checks do we need and how fast can they be made?

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 3 • 12 years ago

Neither CS nor IM can inline through getters/setters yet, but both boil away the property lookup and bake in a call. Inlining through them in IM is blocked on bug 765454.

	Katelyn Gadd (:kael) Reporter
	Comment 4 • 12 years ago

I think some performance penalty is fine here, as there are many cases where you can just replace your property getter/setter accesses with ordinary function calls. But there are use cases where you absolutely need them - lazy evaluation, and interfaces that originally had properties instead of getter/setter functions, for example. I think it's important to support those use cases in the long run.

In the short term it may help to try and educate people about this particular performance issue, since it's hard to spot from profiling in V8 or SpiderMonkey.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 5 • 12 years ago

This would be really nice for API design purposes -- getters/setters can be pretty clean instead of writing functions, so it would be nice to have no perf loss here.

	Anthony Pesch
	Comment 6 • 11 years ago

I just ran into this last night and it took a while to figure out.

I'm working on a game and I'd recently swapped out some of my hand translated libraries with emscripten compiled libraries along with a views lib that wrapped emscripten's heap. The JavaScript views objects contained a lot of Object.defineProperty's to handle reading / writing at the appropriate place in the heap.

This worked fine in the latest Chrome, but ran _terrible_ in FF nightly. After swapping out all of the property definitions with with get_*() and set_*() everything worked great, however it's not very elegant for this application.

	Katelyn Gadd (:kael) Reporter
	Comment 7 • 11 years ago

I should also mention that I am relying on getters/setters for a partial Binary Data shim that I'm using in some of my experiments with emscripten interop and typed array heaps. It's much harder to eliminate the use of getters/setters in this code because I need a way for packed structs backed by a typed array to look and behave like packed structs that live entirely in a JS object. 

It's not possible for my codegen to just figure out 'oh, these are getters/setters' and insert method calls - the only alternative would be to introduce getters/setters for *all* values and then always call the methods, which would likely be a perf loss overall and would complicate the generated code.

In SPS profiles most of the problem seems to be the transition out of the jitcode into GetPropertyOperation/SetPropertyOperation and kin, then back into the actual getter/setter code. I see a lot of time spent there. Inlining would be great too, but I'm not even sure it's necessary - perf might be adequate without it.

	Michael Bebenita [:mbx]
	Comment 8 • 11 years ago

This is also very important for Shumway since it makes heavy use of getters/setters to emulate ActionScript3 semantics.

	Till Schneidereit [:till]
	Comment 9 • 11 years ago

I did some more - shell-based - benchmarking of this.

Using a getter is, for me, exactly 1.5x as slow as either using a method or directly accessing the field: with the attached script, I get 28ms for direct access and method call vs. 42ms for getter access.

This isn't surprising, given that, as per comment 3, method calls are inlined, while getters/setters are not. Once bug 765454 is fixed, we should have speed parity for inlineable (i.e., most) getters/setters.

	Till Schneidereit [:till]
	Comment 10 • 11 years ago

So, there's more to this story - and it's not pretty.

The attached version of the benchmark simply wraps the entire script into a function and executes that. Well, and it unrolls the loops 10x to reduce the influence of loop overhead.

Output on my machine:
direct access: 4
getter access: 1537
method access: 7

That is, the getter version is more than 380x as slow as the field version and more than 200x as slow as the method version.

If I move just the definition of obj to the toplevel, the getter version goes down to 32ms, with the others staying the same.

I'm sure there're some optimizations going on, for the field and method versions. I guess they just hoist the (inlinde, in the method case) field access, leaving only a register access in the loop. Or something.

I don't get why the getter version gets so much slower, however.

I'm sure that there's some optimization going

	Nicolas B. Pierron [:nbp]
	Comment 11 • 11 years ago

(In reply to Kevin Gadd (:kael) from comment #7)
> In SPS profiles most of the problem seems to be the transition out of the
> jitcode into GetPropertyOperation/SetPropertyOperation and kin, then back
> into the actual getter/setter code. I see a lot of time spent there.
> Inlining would be great too, but I'm not even sure it's necessary - perf
> might be adequate without it.

This should not happen, otherwise this means that instead of producing a call, we go through the VM function, which is extremely bad.


(In reply to Till Schneidereit [:till] from comment #10)
> Created attachment 762196 [details]
> Shell benchmark part 2: the rise of the functions
> 
> Output on my machine:
> direct access: 4
> getter access: 1537
> method access: 7

FYI, this is likely related to the fact that IonMonkey does not yet inline getters/setters.  The problem is that we do not have a good way to represent a call issued from a getter/setter and resume it correctly in the baseline code / interpreter.

If we make any modification, they should be near IonBuilder::getPropTryCommonGetter, and identically for setters.

	Eric Faust [:efaust]
	Comment 12 • 11 years ago

With the WIP patch I posted in bug 765454, I am seeing the following numbers from the microbenchmarks above:

Shell Benchmark: The first:

Before:
direct access: 27
getter access: 39
method access: 26

After (with patch):
direct access: 26
getter access: 27
method access: 26


Shell Benchmark Part 2: Electric Boogaloo:

Before:
direct access: 3
getter access: 26
method access: 3

After:
direct access: 3
getter access: 5
method access: 3

Till, the third benchmark you passed me (referenced in comment 10) is so slow because it's interacting with the Ion ICs the whole way, which means vm-based adjudication of the getter at every call. I will investigate that as my next priority.

	Till Schneidereit [:till]
	Comment 13 • 11 years ago

I've long gone to bed and am fast asleep. That can't keep me from quickly saying: Horray!

	Katelyn Gadd (:kael) Reporter
	Comment 14 • 11 years ago

Those are really great numbers. Once this lands, V8 and SpiderMonkey will both have performant properties, a bunch of my data structures will be faster, and I can turn off a whole pessimization pass in my compiler. (:

Let me know if you'd like me to build with that patch applied and test some apps against it.

	Eric Faust [:efaust]
	Comment 15 • 11 years ago

With the new WIP in bug 765454, we add setters!

With the microbenchmark attached, we see the following numbers:

Before:

direct access: 11
setter access: 35
method access: 11

After:

direct access: 11
setter access: 12
method access: 11

So, it seems that we also get good setter times after inlining.

Kevin, can you take the patch for a spin and see what you see? I hope it's stable enough to do whatever you want with it, and I'm interested to see how often we reap benefits in other codespaces that our simple microbenchmarks.

	Katelyn Gadd (:kael) Reporter
	Comment 16 • 11 years ago

Hi, I did a release build of the latest m-c revision with this patch applied, but performance is absolutely miserable (really long GC pauses, too). What mozconfig should I use to test this? Do I need to force PGO on or something?

	Katelyn Gadd (:kael) Reporter
	Comment 17 • 11 years ago

Here's a simple property getter performance test case using JSIL. The results are... weird.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 7 years ago

kael, is this still an issue?  I tried running your big .zip testcase, but I'm not sure what the output from it means or what output is expected....

	Katelyn Gadd (:kael) Reporter
	Comment 20 • 7 years ago

This has been fixed, AFAIK.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 7 years ago

Thank you for double-checking!