1049840 - Add generic GetProp stub to baseline for polymorphic sites

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Johannes Schulte [:jschulte]]

Both a todo in BaselineIC.cpp and jandem in bug 965793, comment 4 suggested to replace the optimized stub chain for getprop with a generic stub, if MAX_OPTIMIZED_STUBS is reached.

Comment 1

[Johannes Schulte [:jschulte]]

Attachment: v1.patch

Comment 2

[Johannes Schulte [:jschulte]]

Attachment: microbenchmark_polymorphism.js

So, on the attached micro-benchmark with ./js --baseline-eager --no-ion -b (average of 5 runs) I get:
pre-patch: 27,7562 ms
with patch: 21,1738 ms
-> 24 % improvement

On Octane-Typescript (again baseline-eager and no-ion, average of 10 runs) I get:
pre-patch: 4828.2
with patch: 4867,5
-> 0.8 % improvement

Comment 3

[Kannan Vijayan [:djvj]]

Comment on attachment 8468727 [details] [diff] [review]
v1.patch

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

Looking at this patch, it seems that the "generic" getprop stub is almost identical to the fallback stub, except that the fallback stub does a bunch of extra checks to see if it can attach a new stub.

I am curious about the difference between this approach, and one where we just rely on the fallback stub.  Instead of clearing all the optimized stubs and attaching a new generic stub, we can just remove all the optimized stubs and set a "generified" flag on the fallback stub that tells it not to attach new optimized stubs.

The main reason I'm concerned about this is the cost overhead of a new VMWrapper for DoGetPropGeneric.  Each of the VMWrapper trampolines for every new native function being called with a vmCall uses up a ton of memory.  It was leading to slow start-up times on B2G.  We want to try to minimize these as much as possible.

Can you try the approach suggested here and see if it gives roughly equivalent results?  If we can avoid a new VMWrapper, that'd be a good thing.

Comment 4

[Johannes Schulte [:jschulte]]

Attachment: v2.patch

Looks good, thanks. With same parameter like above I get:
microbenchmark
pre-patch: 26.8576 ms
with patch: 18.966 ms
-> 29 % improvement

typescript
pre-patch: 4806.7
with patch: 4884.3
-> 1.6 % improvement

One Question: Why do we monitor the result inside DoGetPropFallback. Isn't the typemonitor chain triggered anyway through masm(ICGetProp_Fallback::compiler::generateCode at least looks like this)?

Comment 5

[Kannan Vijayan [:djvj]]

Comment on attachment 8470444 [details] [diff] [review]
v2.patch

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

Awesome!

About the Monitor in the fallback C++ code: Fallback stubs aren't monitored by the TypeMonitor IC chain.  They return directly to the caller, so fallback code has to manually update the relevant typeset from the C++ code, just as the interpreter does.

Comment 6

[Johannes Schulte [:jschulte]]

Attachment: v3.patch

Well, this didn't really work out, the Monitor-Call is apparently to costly.
I get the following results:
microbenchmark:
pre-patch: 26.0322 ms
with patch: 20.5875 ms
-> 21 % improvement
but...
octane-tyescript:
pre-patch: 4738.2
with patch: 4493.6
-> 5 % regression

Doesn't look like this is really an optimization, what do you think?

Comment 7

[Kannan Vijayan [:djvj]]

Well, I think your original patch is the way to go then.  Thanks for trying out my suggestion anyway.

Comment 8

[Johannes Schulte [:jschulte]]

Attachment: v4.patch

Ok, if you think, that's worth the memory cost, let's use my first patch. Nevertheless I would also be fine with not doing anything about this, as the wins aren't too big, so it's up to you.

I also did some further testing on setting MAX_OPTIMIZED_STUBS to 16, as jandem proposed in bug 965793, comment 4 , and the results are promising:
only-baseline typescript:
stock: 4717.8
patch-8: 4758,7
patch-16: 4798

full octane with both jits:
stock: 18040.6
patch-8: 18036.8
patch-16: 18076.8
(all results are the average of 10 runs)

So I'm also attaching a v4 with MAX_OPTIMIZED_STUBS = 16. Your decision, if we pick one at all, and if so, which one :)

Comment 9

[Kannan Vijayan [:djvj]]

Comment on attachment 8474754 [details] [diff] [review]
v4.patch

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

Looks good.  I want an additional ASSERT at the top of DoGetPropFallback, right before the call to |ComputeGetPropResult|.

We should never get to the fallback stub when the generic GetProp stub is attached.  So we should JS_ASSERT that if we're at the fallback stub, that no GetProp_Generic stub is attached:

  JS_ASSERT(!stub->hasStub(ICStub::GetProp_Generic));


r=me with comments addressed.

::: js/src/jit/BaselineIC.cpp
@@ +6534,5 @@
>          return false;
>  
>      if (stub->numOptimizedStubs() >= ICGetProp_Fallback::MAX_OPTIMIZED_STUBS) {
> +        // Discard all stubs in this IC and replace with generic getprop stub.
> +        for(ICStubIterator iter = stub->beginChain(); !iter.atEnd(); iter++) {

Nit: SpiderMonkey style is to not use braces on one-line conditionals.

Comment 10

[Johannes Schulte [:jschulte]]

Attachment: v5.patch

Thanks for the quick review. Could you please do a try-run for this, as I don't have the necessary commit access level?

Comment 11

[Kannan Vijayan [:djvj]]

Pushed to try:

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=fef1c3e7c284
treeherder: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fef1c3e7c284

Comment 12

[Johannes Schulte [:jschulte]]

Oh yes, seems like I have to keep the stack synced for the decompiler. Will work in this, once I'm back on my dev-machine instead of my smartphone :)

Comment 13

[Kannan Vijayan [:djvj]]

Ah!  Yeah.. just take a look at the PopValues(N) arguments passed to VMFunction trampolines in other places in BaselineIC.cpp.

Comment 14

[Johannes Schulte [:jschulte]]

Attachment: v6.patch

PopValues looks like it's only useful for tailcalls. This approach seems to work. Again, if r+, a try-run would be great.

Comment 15

[Kannan Vijayan [:djvj]]

Comment on attachment 8481947 [details] [diff] [review]
v6.patch

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

Looks good.  Good call on the Stow/Unstow thing.  Doing try run now: https://tbpl.mozilla.org/?tree=Try&rev=a75c03a88589

Comment 16

[Johannes Schulte [:jschulte]]

Attachment: v7.patch

Try-run in comment 15.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6aae588b39a7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6aae588b39a7