1041688 - Dynamic definite properties analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

This came out of bug 1038917 and some IRC discussion.  Many of the problems with the current system pointed out in bug 1038917 are inadequacies with the static definite properties analysis.  Mainly:

a) The analysis runs eagerly; when a constructor is first invoked we baseline compile it and possibly other functions it calls, and do a complicated analysis of the scripts.

b) The analysis does a poor job with many real world constructs.  Most things other than straightline code and inlinable functions that add properties to the |this| object will break the analysis.

We could help address both of these by redesigning how the definite properties analysis works, to incorporate both static and dynamic techniques.  The static part could potentially be removed later.  The dynamic technique should be able to mark as definite all properties which all instances of the type are given, regardless of the complexity of that initialization, whether it occurs in the constructor or whether the object escapes into the heap before initialization completes.  Some gotchas listed below, though.  The main additional cost vs. the current approach is that when code deals with partially initialized objects we don't have definite property information for them and the compiled code will be slower.  Memory usage shouldn't change significantly.

The strategy would be as follows:

- When a constructor is first invoked for TypeObject T, don't run the definite properties analysis.  Instead, keep track of all the JSObjects with type T in a structure attached to T.

- After N objects have been created with type T (maybe N == 10 to line up with baselineUsesBeforeCompile), run the combined definite properties analysis via the below steps.

-- Compute a greatest common prefix shape P of the N objects.  So if one has properties {a,b,c} and another has {a,b,d} the prefix is {a,b}.

-- Run the existing static definite properties analysis as is currently done, computing a static shape S.

-- If S == P, the static analysis did as well as it could be expected to do, so make the properties in S/P definite and don't do anything else.

-- If S is not a prefix of P (i.e. S has more properties or different properties than P) then types which were never monitored by baseline have invalidated the definite property information.  This case is strange and hard to trigger but possible if we don't baseline-compile eagerly, so we should just abort the analysis in this case.  For similar reasons we should abort if the constructor is reentrant and has frames on the stack when the N+1th object is created.

-- If S is a prefix of P (i.e. P has more properties than S) then the static analysis missed some properties which all instances of the object have, and we should run the dynamic analysis as follows:

--- Mark all properties in P as definite in T.

--- Create a new type object IT for partially initialized objects of type T.  Any properties in S can be marked as definite in IT, but other properties in P are *not* marked definite.

--- All new objects created by the constructor must now have type IT rather than type T.

--- When an object of type IT is given shape P, its type can be changed to T.  Allowing type changes in this way requires that whenever IT is added to a type set, T also be added.

And that's it.  We keep track of which code deals with partially initialized objects using the current architecture, just by following where values of type IT show up.  If IT escapes into the heap before being initialized, and is read later when it has been fully initialized, we use one of our existing type barriers at that read and are able to use the definite properties in P.  This analysis can still fall over if P is picked badly, though:

- If P has too few properties, then some of the N objects have not been fully initialized.  The normal way this would come up is if the objects are initialized in batches, rather than one at a time.  For this approach to do its best job, all properties should be added to one created object before the next one is created.  In this case the properties in P will all be definite but any additional ones will need shape guards when they are accessed.

- If P has too many properties, then the initialization pattern changed after the initial objects were created.  Since P is never given to later objects, they retain their IT type and we aren't able to optimize accesses to any properties S doesn't have, even if the object has additional properties included in P.

So, this approach isn't perfect but it should be a lot more robust than what we are doing now, and from what I understand from Kannan I don't think the games he's looked at would fall prey to the above issues.  Mainly, with this analysis in place we could just recommend to people that for the best performance they just fully initialize objects one at a time with consistent properties, rather than asking them to write code that conforms to the narrow precepts of the static definite properties analysis.

Comment 1

[Kannan Vijayan [:djvj]]

This all sounds workable, and should improve the robustness of definite properties considerably.

As you noted in the IRC discussion, getprop optimization specialized on incomplete types will likely need to do some kind of hole check for uninitialized properties, but we might even be able to avoid that if we pre-initialize expected future fixed slot positions to Undefined.

The other concern is tracking these deferred-type objects in heap typesets.  The best option for that I can see is to do the lazy-singleton tracking discussed in bug 103897.

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Kannan Vijayan [:djvj] from comment #1)
> As you noted in the IRC discussion, getprop optimization specialized on
> incomplete types will likely need to do some kind of hole check for
> uninitialized properties, but we might even be able to avoid that if we
> pre-initialize expected future fixed slot positions to Undefined.

If the GETPROP is on an object whose type is known to be fully initialized, it will have the definite property and we won't need any new checks.  If the object might have the partially initialized type, then from TI we won't know anything about properties it has other than those the static analysis determined, and optimized code will need to either emit a cache/shape-guard or figure out the shape to use in some other way (e.g. if we start emitting addprops in inline code then we would know the object's shape after doing a bit of analysis).  But, regardless, we shouldn't need to change how GETPROPs or SETPROPs (except ADDPROPs) are optimized in order to preserve correctness.

> The other concern is tracking these deferred-type objects in heap typesets. 
> The best option for that I can see is to do the lazy-singleton tracking
> discussed in bug 103897.

So far as I can tell these are different issues.  We don't need to make any changes to be able to track objects with the partially initialized type as they flow around on the stack and heap; we're just picking a different type for the object when it is created and from there things will just work as normal using the existing machinery.

The only real new wrinkle is being able to change an object from an initial type to a final type.  To do this, we need to make sure that any type set containing the initial type also has the final type, as outlined above (this is simple to do).  One additional thing I forgot is that we also need to make sure the information in the final type captures the contents of the object.  We could do this by applying property type changes on the initial type to the final type as well, so that the property types in the initial type are a subset of the property types in the final type (this is also simple to do).

Comment 3

[Brian Hackett [Laid off!]]

Attachment: WIP

WIP.  This makes a lot of changes to how the new script properties analysis works, following along very closely with comment 0.  This patch works but currently regresses several octane tests so needs some more tuning.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: WIP

Another WIP.  This fixes some perf issues but on octane this still regresses raytrace, box2d and typescript by 5-10%, while improving gameboy by 5-10%.  Other tests don't seem to move.  These regressions have been kind of frustrating to investigate as the metrics I use to look at perf --- quality of the executed Ion code, Ion compilation count, major/minor GC count --- are as good or better with this patch (in the case of box2d codegen, considerably better) yet the score is still worse.  Will try using cachegrind next to see if it offers any insight.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

This patch seems to fix the regressions mentioned above.  We were repeatedly bailing out without recompiling the Ion code, due to an ordering issue when updating type information for objects becoming fully initialized (type information for the partially initialized type was not updated and the type guards we add for Ion SETPROP caches were triggering inappropriately).

I'll break this patch up for review, it's the trickiest one I've written for a long time but makes some pretty nice structural changes.  Besides several simplifications, keeping track of the preliminary objects before doing the new script properties analysis makes it more feasible to store unboxed data in objects: code will be able to run and fill in the preliminary objects so we know what their property types should be, and when we do the new script properties analysis we know all objects in existence with the type and can go fix them up to have a uniform unboxed representation for use in Ion compilation.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: rebase (36ffb9b24c56)

Comment 7

[Brian Hackett [Laid off!]]

Attachment: debugging cleanup

Some debugging cleanup:

1) Include a description with -D output for blocks from inlined frames, to make the output easier to use.

2) Fix a crash when calling TypeCompartment::print from gdb.

This can land before the other patches.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: allow TypeObject pre barriers

A later patch will need to overwrite TypeObject pointers from jitcode.  This patch allows pre barrier jitcode calls to be performed on such pointers, and cleans up the JIT pre barrier calls mechanism in the process.  This can land before the other patches.

Comment 9

[Brian Hackett [Laid off!]]

Attachment: Don't create 'this' objects multiple times

Currently, on some paths through the interpreter's invoke mechanism we end up creating |this| values for a |new| call multiple times.  This breaks the acquired properties analysis, which needs to know that all objects of a type have a particular shape, and when an object is discarded like this it will never have properties added to it.

This patch fixes the interpreter so redundant |this| values are not created, and cleans up some related duplicate code.  This patch can land before the other patches.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: main patch

The main patch in this series, implements the acquired properties analysis and the new script properties analysis (umbrella analysis controlling the other two).  This patch excludes the changes made to ADDPROP operations required by the former, which will be in an uncoming patch.

Comment 11

[Brian Hackett [Laid off!]]

Attachment: ADDPROP changes

This patch has the changes needed to the VM and JITs for ADDPROP operations, as well as JIT changes to make sure incorrect 'new' objects are not baked in when compiling before the new script analysis has been performed.

Comment 12

[Brian Hackett [Laid off!]]

Attachment: Eagerly analyze scripts during Ion compilation

With the previous two patches, if Ion compiles accesses to objects before the new script properties analysis has been performed, it will not get any definite property information.  This situation happens pretty often when only a few objects of a type are created, so this patch changes things so that objects accessed during Ion compilation have the new script properties analysis immediately performed on them.

Comment 13

[Brian Hackett [Laid off!]]

Attachment: Watch for baseline ICs on preliminary objects

Before the new script properties analysis has been performed, objects of a type have the maximum number of fixed slots.  After, they might have fewer, and if baseline code sees both the old and new objects we will end up with a polymorphic cache even if the behavior is subsequently monomorphic.  This ends up hurting Ion perf on some octane tests, so this patch has the baseline inspector ignore cache entries that were generated for preliminary objects.  This isn't perfect, as if after the analysis is performed the objects still have the max number of fixed slots, the old cache entries will still hit, but this patch fixes the regression I observed.

Comment 14

[Brian Hackett [Laid off!]]

Attachment: remove JSContext in IonBuilder

Since we don't perform the new script properties analysis until after 20 or so objects have been created, scripts which are called by the constructor should have already been baseline compiled and had their type sets populated, removing the need to do this stuff eagerly during the definite properties analysis.  This patch removes the JSContext used by IonBuiler in such cases, and is a nice cleanup.  While this can affect behavior --- if we analyze before baseline compilation due to being eagerly triggered by IonBuilder, we won't end up getting any definite properties --- this doesn't seem to affect octane perf.  The main benefit of doing the definite properties analysis right now is that we can optimize addprop for those definite properties, rather than using an IC, so this only helps in constructors that are called a lot.  This patch can land after the other patches.

Comment 15

[Brian Hackett [Laid off!]]

Attachment: remove dynamic template object resizing

Currently, we support dynamically resizing template objects for 'new' script types, since we compute the template before the constructor has even executed.  With these patches, we don't compute the template until after some preliminary objects have been created, so the template can be sized to reflect properties not found by the definite properties analysis.  With this change, the dynamic resizing mechanism is unnecessary and can be removed.  This patch can land after the other patches.

Comment 16

[Brian Hackett [Laid off!]]

Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec4622e6e1fa

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8477142 [details] [diff] [review]
allow TypeObject pre barriers

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

::: js/src/jit/CodeGenerator.cpp
@@ +1650,5 @@
>      int32_t offset = lir->mir()->slot() * sizeof(js::Value);
>      Address dest(base, offset);
>  
>      if (lir->mir()->needsBarrier())
> +        emitPreBarrier(dest, MIRType_Value);

AFAICS CodeGeneratorShared::emitPreBarrier is always called with second argument = MIRType_Value. If that's the case maybe just remove it.

::: js/src/jit/VMFunctions.h
@@ +717,5 @@
>  JSObject *TypedObjectProto(JSObject *obj);
>  
> +void MarkValueFromIon(JSRuntime *rt, Value *vp);
> +void MarkShapeFromIon(JSRuntime *rt, Shape **shapep);
> +void MarkTypeObjectFromIon(JSRuntime *rt, types::TypeObject **typep);

Please move the definitions from Ion.cpp to VMFunctions.cpp as well.

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8477147 [details] [diff] [review]
Don't create 'this' objects multiple times

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

Nice to have this factored out.

::: js/src/jit/Ion.cpp
@@ -2261,5 @@
> -            RootedObject obj(cx, CreateThisForFunction(cx, callee,
> -                                                       invoke.useNewType()
> -                                                       ? SingletonObject
> -                                                       : GenericObject));
> -            if (!obj || !jit::IsIonEnabled(cx)) // Note: OOM under CreateThis can disable TI.

Yay bug 972817.

::: js/src/vm/Interpreter.cpp
@@ +359,5 @@
> +    if (isInvoke()) {
> +        InvokeState &invoke = *asInvoke();
> +        if (invoke.constructing() && invoke.args().thisv().isPrimitive()) {
> +            // Keep the GC from wiping out jitcode that is about to execute.
> +            AutoSuppressGC suppress(cx);

This is not strictly necessary for correctness as we call this before checking if there's a Baseline/Ion script right?

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 8477152 [details] [diff] [review]
main patch

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

Looks great. The comments and many asserts are really useful.

::: js/src/jsinfer.cpp
@@ +2785,5 @@
>  {
>      if (unknownProperties())
>          return true;
>  
>      /* Mark all properties of obj as definite properties of this type. */

Nit: s/obj/shape

@@ +3536,5 @@
> +        }
> +    }
> +
> +    // There should be room for registering the new object.
> +    MOZ_CRASH();

Nit: you can use MOZ_CRASH("There should..."), that's also useful for the fuzzers.

@@ +3721,5 @@
> +
> +    RootedTypeObject typeRoot(cx, type);
> +    templateObject_ = NewObjectWithType(cx, typeRoot, cx->global(), kind, MaybeSingletonObject);
> +    if (!templateObject_)
> +        return false;

This function returns |false| both on OOM or other exceptions and when analysis failed. Shouldn't we propagate the former?

@@ +3801,5 @@
> +    if (!initialType)
> +        return false;
> +
> +    if (!initialType->addDefiniteProperties(cx, templateObject()->lastProperty()))
> +        return false; 

Nit: trailing whitespace

::: js/src/jsinfer.h
@@ +830,5 @@
> +// the properties which that object will eventually have. This is done via two
> +// analyses. One of these, the definite properties analysis, is static, and the
> +// other, the acquired properties analysis, is dynamic. As objects are
> +// constructed using 'new' on some script to create objects of type T, our
> +// analysis strategy is as follows:

Nice and clear comment. One thing that wasn't clear after reading it is what we discussed on IRC: if < PRELIMINARY_OBJECT_COUNT objects are allocated, Ion can still use the acquired properties analysis and still has definite property information. Please explain this somewhere.

Comment 20

[Brian Hackett [Laid off!]]

Part 2 (TypeObject pre barriers):

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf2bf138571c

Comment 21

[Brian Hackett [Laid off!]]

Part 3 (new object creation):

https://hg.mozilla.org/integration/mozilla-inbound/rev/870d25b5d7b3

The AutoSuppressGC is unnecessary, yeah, and I removed it.  It was a holdover from when I was trying to create the 'this' object in a different place.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8477157 [details] [diff] [review]
ADDPROP changes

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

r=me with comment below addressed.

::: js/src/jit/IonCaches.cpp
@@ +2880,5 @@
>  
> +    if (inlinable) {
> +        // The property did not exist before, now we can try to inline the property add.
> +        bool checkTypeset;
> +        if (!addedSetterStub && canCache == MaybeCanAttachAddSlot &&

The |if (inlinable)| is unnecessary as canCache will always be CanAttachNone if inlinable == false. Can you kill "bool inlinable" completely and use "cache.canAttachStub() && !obj->watched()" as condition of the if at the start of the function?

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 8477159 [details] [diff] [review]
Eagerly analyze scripts during Ion compilation

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

::: js/src/jit/IonBuilder.cpp
@@ +740,5 @@
>          return false;
>  
> +    if (abortedNewScriptPropertiesType()) {
> +        abortReason_ = AbortReason_NewScriptProperties;
> +        return false;

Can this happen during the DPA or arguments analysis? If not we should probably assert it.

@@ +8269,5 @@
> +{
> +    if (!types || types->unknownObject())
> +        return UINT32_MAX;
> +
> +    uint32_t slot = UINT32_MAX;

Nit: move this after the first loop, as it's only used in the second loop.

@@ +8289,5 @@
> +            continue;
> +
> +        if (types::TypeObject *typeObject = type->maybeType()) {
> +            if (typeObject->newScript() && !typeObject->newScript()->analyzed()) {
> +                abortedNewScriptPropertiesType_ = typeObject;

What if there are multiple Typeobjects that still need to be analyzed, do we risk running IonBuilder N times? I also wanted to suggest aborting compilation immediately instead of continuing, but it might be better to continue and maintain a Vector of TypeObjects that have to be analyzed?

I'm also wondering if just running the analysis here and adding the JSContext back to IonBuilder might be simpler and faster...

Comment 24

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #23)
> What if there are multiple Typeobjects that still need to be analyzed, do we
> risk running IonBuilder N times? I also wanted to suggest aborting
> compilation immediately instead of continuing, but it might be better to
> continue and maintain a Vector of TypeObjects that have to be analyzed?

Yeah, I thought about doing this but got lazy.  I'll add a vector of type objects to analyze at the end.

> I'm also wondering if just running the analysis here and adding the
> JSContext back to IonBuilder might be simpler and faster...

The problem here is that the new script properties analysis can change type information --- when we run IonBuilder it can optimistically add new observed types, and when finishing the analysis we can add properties to type objects, which can cause the type to be marked unknown.  Allowing this in the middle of IonBuilder could lead to weird and potentially incorrect behavior.

Comment 25

[Brian Hackett [Laid off!]]

Attachment: Eagerly analyze scripts during Ion compilation (v2)

Also, I added an assert that we don't abort during analysis, this isn't possible since we don't try to optimize property accesses when building for the definite properties or arguments usage analyses.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec4622e6e1fa
https://hg.mozilla.org/mozilla-central/rev/bf2bf138571c
https://hg.mozilla.org/mozilla-central/rev/870d25b5d7b3

Comment 27

[Kannan Vijayan [:djvj]]

(In reply to Brian Hackett (:bhackett) from comment #24)
> The problem here is that the new script properties analysis can change type
> information --- when we run IonBuilder it can optimistically add new
> observed types, and when finishing the analysis we can add properties to
> type objects, which can cause the type to be marked unknown.  Allowing this
> in the middle of IonBuilder could lead to weird and potentially incorrect
> behavior.

If this does happen, it should be detectable if the typeinfo changes are incompatible with compilation decisions that have already been made, simply by looking for triggering of constraints already attached during the compile, no?

I.e. if IonBuilder runs, and does a bunch of NewScript analyses, but at the end of the compile none of the attached constraints have been invalidated, then the compile is still valid, no?  If this is only true in theory and not actually true in practice the vast majority of the time, though.. then it's probably not worth it.

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8478335 [details] [diff] [review]
Eagerly analyze scripts during Ion compilation (v2)

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

Thanks for addressing that.

Comment 29

[Jan de Mooij [:jandem]]

Comment on attachment 8477169 [details] [diff] [review]
remove dynamic template object resizing

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

That's removing some complicated code.

Comment 30

[Brian Hackett [Laid off!]]

(In reply to Kannan Vijayan [:djvj] from comment #27)
> If this does happen, it should be detectable if the typeinfo changes are
> incompatible with compilation decisions that have already been made, simply
> by looking for triggering of constraints already attached during the
> compile, no?
> 
> I.e. if IonBuilder runs, and does a bunch of NewScript analyses, but at the
> end of the compile none of the attached constraints have been invalidated,
> then the compile is still valid, no?  If this is only true in theory and not
> actually true in practice the vast majority of the time, though.. then it's
> probably not worth it.

Well, yeah, the IonBuilder design is pretty robust against this sort of change, mainly because the changes made for off thread IonBuilder required that we be able to tolerate type changes on the main thread while IonBuilder proceeds off thread.  But at certain points within the compilation of a single op though we could get type changes if we ran the new script properties analysis, which isn't possible even with off thread IonBuilder.  It's just hard to say if this would cause a problem or not; it's just a cleaner design and easier to reason about when IonBuilder can't change any type information itself.

Comment 31

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #13)
> This ends up hurting Ion perf on some octane tests, so this patch has the
> baseline inspector ignore cache entries that were generated for preliminary
> objects.  This isn't perfect, as if after the analysis is performed the
> objects still have the max number of fixed slots, the old cache entries will
> still hit, but this patch fixes the regression I observed.

I'm a bit worried that ignoring certain Baseline caches/shapes can cause us to keep bailing out due to shape guard failures. We've had a number of these "repeated bailouts" bugs and they are really annoying and can destroy performance :(

Comment 32

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #31)
> I'm a bit worried that ignoring certain Baseline caches/shapes can cause us
> to keep bailing out due to shape guard failures. We've had a number of these
> "repeated bailouts" bugs and they are really annoying and can destroy
> performance :(

Well, an alternate design I thought of would be to remove the baseline caches from within baseline itself, rather than by ignoring them in the baseline inspector.  We still keep the bit on the caches indicating whether they are for preliminary objects, and whenever we attach a getprop/setprop stub which isn't on a preliminary object to a baseline cache, we clear out any cache entries which are on preliminary objects.

Comment 33

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #32)
> Well, an alternate design I thought of would be to remove the baseline
> caches from within baseline itself, rather than by ignoring them in the
> baseline inspector.  We still keep the bit on the caches indicating whether
> they are for preliminary objects, and whenever we attach a getprop/setprop
> stub which isn't on a preliminary object to a baseline cache, we clear out
> any cache entries which are on preliminary objects.

Yeah, that sounds reasonable and safer. That way when we see a preliminary object later on, we will bailout and attach a new stub. If you can post a new patch I'll review today or tomorrow (since it's the last blocker as far as I can see).

Comment 34

[Brian Hackett [Laid off!]]

Attachment: preliminary object baseline ICs (v2)

Comment 35

[Brian Hackett [Laid off!]]

Main patch, everything that hasn't landed yet except the IonBuilder JSContext removal:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b25f21fe08

Comment 36

[Kannan Vijayan [:djvj]]

There seems to be a richards regression (7.4%) on AWFY MacOS x86-64 that's a bit sticky.  Otherwise, there are some nice wins in several octane benches.

Comment 37

[Dan Gohman [:sunfish]]

I'm seeing the following abort on x64, which bisected to a1b25f21fe08:

$ cd js/src/octane
$ js run-box2d.js 
Assertion failure: MIR instruction returned value with unexpected type, at js/src/jit/IonMacroAssembler.cpp:1339
Trace/breakpoint trap

Comment 38

[Brian Hackett [Laid off!]]

Attachment: populate singleton temporary type sets

This fixes the problem in comment 37 for me.  When making singleton type sets during compilation we weren't ensuring that type sets containing an initial type also had the fully initialized type.

Comment 39

[Brian Hackett [Laid off!]]

Oh, and I'll look and see if I can reproduce that richards regression tomorrow (landed this at the beginning of the cycle so there would be more time to sort out regressions).

Comment 40

[Brian Hackett [Laid off!]]

Singleton type set fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b20a4e9ce15

Comment 41

[Brian Hackett [Laid off!]]

I think the richards regression is a problem with our inlining heuristics.  Since we baseline compile constructors at different times now, use counts and compilation orders have been perturbed a bit.

Before the main patch landed, I get a score of 24800 on richards (all numbers are on a 10.9 x64 build, averaged over several runs).

After the main patch landed, I get a score bimodally of either 20000 or 23000.

If I then disable the vetoing mechanism for insufficiently hot callees ("Callee must have been called a few times to have somewhat stable type information"), my score improves to 25500.

If I disable the above vetoing mechanism in a tree from before the main patch landed, my score stays around 24800.

I'm not sure how much sense this veto mechanism makes.  When a script's Ion code is discarded its use count is reset, though its type information is retained.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1b25f21fe08
https://hg.mozilla.org/mozilla-central/rev/7b20a4e9ce15

Comment 43

[Brian Hackett [Laid off!]]

IonBuilder JSContext removal:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce0afdc6ea1f

Comment 44

[Brian Hackett [Laid off!]]

Backout of ce0afdc6ea1f:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4322addcf997

This caused several regressions on AWFY.  This is a simplification patch which seems to have oversimplified things (it does make the definite properties analysis less powerful) and I don't think it's really worth pursuing right now.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

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