IonMonkey: Set typeset for MCreateThisWithProto

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Blocks: 870627
(Assignee)

Comment 1

4 years ago
Created attachment 758282 [details] [diff] [review]
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.
Assignee: general → hv1989
(Assignee)

Updated

4 years ago
Attachment #758282 - Attachment is patch: true
(Assignee)

Comment 2

4 years ago
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).
Attachment #758282 - Flags: review?(bhackett1024)
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.
Attachment #758282 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/436c88ed1e5e
https://hg.mozilla.org/mozilla-central/rev/436c88ed1e5e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Assignee)

Updated

4 years ago
Depends on: 893853
You need to log in before you can comment on or make changes to this bug.