879168 - IonMonkey: Set typeset for MCreateThisWithProto

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

The benchmark GWT binary trees has no information about the "this" type, but that information is available during createThisScripted. This information was previously available in analyzeTypes. So this regressed benchmarks using MCreateThisWithProto (instead of MCreateThisWithTemplate).

Spicing up the types will improve the binary trees benchmarks from 11s to 2.5s

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Took longer than expected, but register allocation wanted me to study his code. As reported this improves the benchmark to 2.5s. Not putting up for review yet, since I haven't run any tests on it.

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 758282 [details] [diff] [review]
Patch

This passes jit-tests. The extra line in Lowering is needed to support box -> TypeBarrier -> unbox sequence. Since TypeBarrier just redefines the register, it takes the switched vreg from box (type/payload are switched).

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 758282 [details] [diff] [review]
Patch

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

Sorry for the delay.

::: js/src/ion/IonBuilder.cpp
@@ +3411,5 @@
>  
> +    // Improve type information of |this| when not set.
> +    if (callInfo.constructing() && !callInfo.thisArg()->resultTypeSet()) {
> +        types::StackTypeSet *types = types::TypeScript::ThisTypes(calleeScript);
> +        MTypeBarrier *barrier = MTypeBarrier::New(callInfo.thisArg(), cloneTypeSet(types), Bailout_Normal);

I think it would be better to use MMonitorTypes instead of MTypeBarrier, which doesn't have the same funky handling of its inputs.  In bug 804676 I tried to extend MTypeBarrier so that it could handle MBox inputs and eventually just gave up.  The change to Lowering-shared-inl.h below is not I think sufficient to make this work.

Comment 4

[Hannes Verschore [:h4writer]]

I only see downsides when using MMonitorTypes. MMonitorTypes does exactly the same as MTypeBarrier, except it isn't moveable/hoistable and it doesn't redefines it's input (and narrowing the typeset, only tests the typeset).

I'm also pretty sure that this is the only change needed to get MBox to play nice with TypeBarrier. So that would be great, especially if you say you wanted that originally for bug 804676. So with this change we could ditch MMonitorTypes, since that is actually an almost exact copy of MTypeBarrier :D.

Without objection, I'm gonna push this (as is) Tuesday.

Comment 5

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/436c88ed1e5e

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/436c88ed1e5e