I was looking into the dart2js richards benchmark today. There are a number of issues caused by bad type information (dart2js creates prototype objects dynamically, so they all have the same group). We're doing polymorphic inlining of the run$1 function, but we don't specialize the |this| types within each inlined callee, so accessing this-properties isn't fast. This is a result of the bad type information mentioned earlier, but we should be able to do something smarter.

Needinfo'ing myself because this benchmark is slower with unboxed objects.

I think we should be able to handle the case of prototype objects being dynamically created in a more flexible way. I still need to think about the best way to do this, though. If I make sure the prototype objects in this benchmark (|object in inheritFrom and |newDesc| in processClassData) end up as singletons, and bump inlineMaxBytecodePerCallSiteOffThread so that we inline IdleTask.run$1, my score improves from 1800ms to 1250ms (my ca. 10/2014 d8 is 1120ms). --unboxed-objects is still slow though, 1780ms, which I need to look at (the generated Ion code looks good so it's presumably something else).

This patch removes about 2/3 of the --unboxed-objects regression.

This patch allows objects to be marked as singletons at any point after creation, even once they have escaped into the heap. Their old group is given unknown properties, but their properties can be tracked precisely by TI. This improves my time on this test from 1840us to 1330us, when running without unboxed objects. This is a pretty simple change to make, actually, but most of the complexity in the patch is because we can now have singletons that are in the nursery, and which can show up in type sets and require post barriers. These nursery objects can also be used during Ion compilation, and leak to many more places than are handled by MNurseryObject. The patch addresses this by tracking whether there are any type sets anywhere which contain nursery objects; if so, the next minor GC cancels all off thread compilations before touching anything in the heap. This shouldn't have much of a perf cost because prototype objects are normally created early in a program's execution so we should only have to worry about canceling compilations during one of the initial minor GCs. If we run into a testcase that keeps manufacturing prototype objects, though, we could pretenure any group that has had objects converted to singletons in the past (though that would have its own perf costs). With this patch, I think that our handling of nursery pointers in the JIT is pretty incoherent. We have ImmGCPtr, which can't be in the nursery, ImmMaybeNurseryPtr, which is basically the same but checks for nursery pointers and sets a flag somewhere, and the MNurseryObject stuff. I think that ImmMaybeNurseryPtr should be removed and that ImmGCPtr handle nursery pointers and make sure post barriers are triggered properly. Otherwise, figuring out what should be ImmGCPtr and what should be ImmMaybeNurseryPtr is just error prone busywork. MNurseryObject can just be removed I think (though the cancel-compilations-on-minor-GC flag will need to be set in another place), it's largely redundant with the stuff in this patch.

(In reply to Brian Hackett (:bhackett) from comment #6) > MNurseryObject can just be removed I think > (though the cancel-compilations-on-minor-GC flag will need to be set in > another place), it's largely redundant with the stuff in this patch. I really liked that MNurseryObject didn't have to cancel any offthread compilations. IMO cancelling compilations is OK when we know it doesn't happen too often, but as you said, a test that keeps creating prototype objects will break this assumption and will cause another perf cliff. I know the patch does fix another perf issue, that's really cool and may well justify this. I'm just wary of adding another cancel/abort/retry mechanism; it may work great on our benchmarks but there always is some stupid benchmark, game or website out there that behaves differently and manages to hit these bad cases. Also, it'll be tempting to use it for more (and more common) things. Can you measure how often we have to cancel compilations on our benchmarks and some popular websites (Gmail, Google Docs/Maps)? (Also, we can't just remove the !IsInsideNursery from MConstant's constructor and similar places. These asserts are incredibly useful and the least we should do is replace then with asserts that check the StoreBuffer flag.)

(In reply to Jan de Mooij [:jandem] from comment #7) > I really liked that MNurseryObject didn't have to cancel any offthread > compilations. I agree, but this change can cause nursery objects to appear in many other places which will be accessed off thread, particularly in type sets. The reason why I think that MNurseryObject is redundant with this change is that we mainly (only?) use MNurseryObject when an object has a nursery proto, and that is exactly the time when we will try to make that prototype a singleton and allow it to appear directly in type sets. > Can you measure how often we have to cancel compilations on our benchmarks > and some popular websites (Gmail, Google Docs/Maps)? I measured the following quantities: A: Minor GCs that don't cancel B: Minor GCs that cancel C: Compilations canceled by minor GCs D: Compilations canceled in other ways Running octane: A: 11849 B: 8 C: 0 D: 105 Browing google maps for a bit: A: 23 B: 0 C: 0 D: 8 Looking through some emails on gmail: A: 29 B: 5 C: 0 D: 8 So I haven't seen any cases where this actually causes us to cancel more in progress compilations. There are already ways in which we can get pathological behavior from type changes causing compilations to continually be canceled, and this doesn't really change that IMO. > (Also, we can't just remove the !IsInsideNursery from MConstant's > constructor and similar places. These asserts are incredibly useful and the > least we should do is replace then with asserts that check the StoreBuffer > flag.) Also agreed, but checking the store buffer flag asynchronously won't work since it isn't protected by any lock, so we'll need some TLS for this check. I can add that, but again I think there is a bigger problem with the meaningless distinction of ImmGCPtr vs. ImmMaybeNurseryPtr, and would like to add TLS in concert with merging these things.

Revised patch that checks constraints on when nursery pointers can be used in MIR and generated code, merges ImmMaybeNurseryPtr and ImmGCPtr, and removes the NurseryObject machinery.

Comment on attachment 8614969 [details] [diff] [review] patch Review of attachment 8614969 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Thanks for adding these asserts! ::: js/src/jit/IonBuilder.cpp @@ +12979,5 @@ > + // GC. All constants used during compilation should either go through this > + // function or should come from a type set (which has a similar barrier). > + if (v.isObject() && IsInsideNursery(&v.toObject())) { > + JSContext* cx = GetJitContext()->cx; > + cx->runtime()->gc.storeBuffer.setShouldCancelIonCompilations(); I think it'd be nice to add a method to CompileRuntime so that you don't need the JSContext (that method could MOZ_ASSERT(onMainThread()). ::: js/src/jit/arm/Assembler-arm.cpp @@ +811,1 @@ > MOZ_ASSERT(!(uintptr_t(ptr) & 0x1)); Maybe just remove the assert now? Or s/0x1/0x7/ to make it more effective. Same for the other archs. ::: js/src/vm/ObjectGroup.cpp @@ +511,5 @@ > + > + // Objects which are prototypes of one another should be singletons, so > + // that their type information can be tracked more precisely. Limit > + // this group change to plain objects, to avoid issues with other types > + // of singletons like typed arrays. Maybe also update this comment in ObjectGroup.h to mention this new singleton case: * Object groups which represent at most one JS object are constructed lazily. * These include groups for native functions, standard classes, scripted * functions defined at the top level of global/eval scripts, and in some * other cases.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e71f980245 Win8 debug devtools-3 was the most cooperative about crashing, https://treeherder.mozilla.org/logviewer.html#?job_id=10756224&repo=mozilla-inbound in 3 of 10 runs, but I suspect https://treeherder.mozilla.org/logviewer.html#?job_id=10758776&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=10758846&repo=mozilla-inbound too.

Looks like mostly an improvement: http://arewefastyet.com/regressions/#/bug/1162986 I only see FluidMotion on 64bit only and dromaeo-object-array. Might be interesting to have a look at the dromaeo-object-array.

According to awfy, this benchmark needs more work.

The leave-open keyword is there and there is no activity for 6 months. :bhackett, maybe it's time to close this bug?