Closed Bug 785905 Opened 12 years ago Closed 10 years ago

IonMonkey: off thread MIR construction

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox29 --- wontfix
firefox30 --- wontfix

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

(Whiteboard: [ion:t])

Attachments

(16 files, 5 obsolete files)

220.73 KB, patch
Details | Diff | Splinter Review
28.01 KB, patch
jandem
: review+
Details | Diff | Splinter Review
46.17 KB, patch
jandem
: review+
Details | Diff | Splinter Review
40.81 KB, patch
jandem
: review+
Details | Diff | Splinter Review
5.73 KB, patch
jandem
: review+
Details | Diff | Splinter Review
66.04 KB, patch
jandem
: review+
Details | Diff | Splinter Review
8.52 KB, patch
jandem
: review+
Details | Diff | Splinter Review
9.46 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.41 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.76 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.18 KB, patch
jandem
: review+
Details | Diff | Splinter Review
4.21 KB, patch
jandem
: review+
Details | Diff | Splinter Review
925 bytes, patch
jandem
: review+
Details | Diff | Splinter Review
2.44 KB, patch
jandem
: review+
Details | Diff | Splinter Review
18.42 KB, patch
jandem
: review+
Details | Diff | Splinter Review
215.07 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
Currently, with bug 785494 and bug 785761 applied, off thread compilation on SS spends 9ms on the main thread and 14ms on the compilation thread (this is in addition to miscellaneous thread overhead, which seems to be <1ms).  4ms of the time on the main thread is code generation (bug 785762), the remaining 5ms is in IonBuilder.

It would be nice to run IonBuilder off the main thread too.  Doing this plus bug 785762 would make triggering off thread Ion compilation extremely cheap for the main thread, and should make compiling Ion code eagerly feasible (i.e. keep a core continually busy with Ion compilation).

Running IonBuilder off the main thread should be a good deal simpler after bug 778724 than before.  Bug 778724 changes how freeze constraints work so that type sets for stack contents in each script are all implicitly frozen, without requiring the compiler to figure out exactly what information it will need and explicitly adding constraints for them (the latter must happen on the main thread).  Constraints on heap type sets (object properties and call return values) must still be added on the main thread, so a prepass will still be needed to generate these (this includes making inlining decisions), but I don't think much restructuring will be needed for the builder.  The inference code itself would need a bit of restructuring to make sure that Ion compilation is cancelled before the stack type sets are changed, not after.
Attached patch part 1 (7566291ca483) (obsolete) — Splinter Review
This removes FreezeStack constraints, and updates all the points where input arg/local/observed type sets for a script are updated to first mark the script as needing recompilation.  The problem with the FreezeStack constraints is that they aren't triggered until after the type set itself is updated.  After this patch, if AddPendingRecompile immediately cancels off thread Ion compilation for a script then we are guaranteed that compilation will not occur when the type sets are updated, and the compiler can read the contents of stack type sets race free.
Assignee: general → bhackett1024
Attachment #656464 - Flags: review?(luke)
Attached patch GetPropertyCache fix (obsolete) — Splinter Review
IonMonkey branch fix so GetPropertyCache doesn't pollute types when used from JSOP_INSTANCEOF.  Before part 1, this Monitor call would just be ignored (the opcode is not JOF_TYPESET), which doesn't seem like very good semantics.  This also works around the correctness issue in bug 787050.
Attachment #656850 - Flags: review?(luke)
Comment on attachment 656464 [details] [diff] [review]
part 1 (7566291ca483)

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

This seems like a good change; any idea how much memory it saves by not creating all those freeze constraints?  It seems prone to bugs since you need to find every possible place where a freeze constraint used to be and instead ensure that the script is recompiled manually.  Is there already a strong assertion to catch these type of bugs?  If not, I had an idea: keep the freeze contraints around and assert, in TypeConstraintFreezeStack::newType, that the script has already been marked for recompilation.

::: js/src/jsinfer.cpp
@@ +539,5 @@
> +    add(cx, cx->analysisLifoAlloc().new_<TypeConstraintSubsetExternal>(target, targetScript));
> +}
> +
> +void
> +HeapTypeSet::addSubsetExternal(JSContext *cx, TypeSet *target, JSScript *targetScript)

Could these be a single method on TypeSet?

@@ +2505,5 @@
>          /* Ignore this barrier, just add the type to the target. */
> +        if (!target->hasType(type)) {
> +            AddPendingRecompile(cx, targetScript, NULL);
> +            target->addType(cx, type);
> +        }

Why not AddExternalType?

::: js/src/jsinfer.h
@@ +460,5 @@
>  
>      /* Constraints for type inference. */
>  
>      void addSubset(JSContext *cx, TypeSet *target);
> +    void addSubsetExternal(JSContext *cx, TypeSet *target, JSScript *targetScript);

I understand the need for a word to indicate "the version that recompiles b/c there isn't a freeze constraint", but "external" is pretty ambiguous ("external to what from where?").  Can you use addRecompilingSubset and (in jsinfer.cpp) s/AddTypeExternal/AddRecompilingType/?
Attachment #656464 - Flags: review?(luke) → review+
Attachment #656850 - Flags: review?(luke) → review+
> but I don't think much restructuring will be needed for the builder

Hahahaha.

This patch factors IonBuilder::build(cx) and everything below it into an IonBuilder::analyze(cx) which runs on the main thread and an IonBuilder::build() which can run off thread.  Time for analysis is maybe 25% of the time for building, so this works pretty well for shifting the remaining work off thread.
Attachment #658072 - Flags: review?(dvander)
Attached patch part 3 (obsolete) — Splinter Review
Some tuning fixes for performance bugs in parallel compilation: IonBuilder aborts were not necessarily causing compilation to be disabled on the script, and canceling an off thread compilation would not always cause a script's use count to be reset.  This also adds a --ion-uses-before-compile knob for specifying how hot to let scripts get before compiling.

SS numbers I get with a threadsafe x86 build:

Stock: 294ms, 20ms spent compiling
Parallel compilation: 279ms, ~20ms compiling off thread, 1.5ms compiling main thread
Parallel compilation, 1000 use threshold: 293ms, ~35ms compiling off thread, 3.5ms compiling main thread

Main thread compilation time includes all time under TestCompiler (analysis prepass), AttachFinishedCompilations (final IonCode generation), and CancelOffThreadIonCompile.  Takeaways here for me are that parallel compilation currently improves score by about 5.5%, 90%+ of compilation time is spent off thread, and compiling more aggressively hurts performance far in excess of the extra main thread time taken.

The last point is my biggest concern; ideally, things should run strictly faster once extra compilation time has been discounted.  This could be due to either Ion code being slower than JM+TI, slowness in the interface between the two compilers (e.g. I have seen slow calls being taken when a JM full call IC stub is calling into Ion code), or something else entirely.  I don't have time to properly investigate this before leaving.
Attachment #658261 - Flags: review?(dvander)
Attached patch patch bomb (dd9554c236dc) (obsolete) — Splinter Review
Patch rolling up all the pending parallel compilation patches, for anyone wanting to try this out.  This includes the patches in this bug, bug 785494, bug 785761, bug 786146, bug 783464 (which was not yet in dd9554c236dc but is needed here), and bug 785762.
Attachment #658072 - Flags: review?(dvander)
Attachment #658261 - Flags: review?(dvander)
I'd like to resurrect this bug.  Currently we run IonBuilder on the main thread and it is a significant source of time (haven't measured exactly how much) and kind of inhibits us from doing Ion compilation more aggressively and potentially from doing Ion recompilation (e.g. recompile very very hot scripts with more aggressive optimizations and better regalloc).

The main reason this didn't get in is because part 2 is a massively invasive trainwreck.  That patch worked (or didn't) by splitting IonBuilder into two phases, an analysis phase and a MIR construction phase.  The second phase worked entirely using information accumulated during the first part, and it is a mess.  Following this approach now, with bug 804676 in place, would be substantially worse, as we need MIR to have basic information about what types can flow to a given opcode.

Fortunately, the baseline compiler will I think let us cut this knot.  The new approach would be:

- When the main thread wants to trigger an Ion compile, it triggers the compilation but does essentially no preparatory work.

- The compilation thread takes the script to be compiled and does all the work off thread, up to CodeGenerator::link.  All examination of VM state happens off thread.

- The only things the compilation thread can look at are stack/heap type sets and baseline stubs attached to a script.  No direct inspection of objects or other mutable state is allowed.

This design has a couple main aspects:

- The main thread never needs to worry about racing when doing typical VM stuff like updating objects, as the compilation thread will never inspect them.

- The main thread and compilation thread need to take a lock when using type sets and baseline stubs.  These operations should be relatively rare and can use the exclusive access lock, which is basically free for the main thread when there is no active compilation.  Can be smart about this and only write to type sets and baseline stubs when on the main thread, so that the main thread also doesn't need to take a lock when reading these things.

Needing more locking on the main thread is a bit unfortunate but I think the cost will be pretty low and will be substantially outweighed by the benefits of zero cost triggering of Ion compilations.
Depends on: 906788
Related to this work: do you have any plans to make IonContext safer with the new context types?  Given how easy it is to GetIonContext()->{cx,runtime,compartment} anywhere from a worker thread, this seems like a really big hole in all the great type safety work you've been doing recently.

I haven't thought through it, but a first guess would be something like:

  class IonExclusiveContext {
    ExclusiveContext *cx();
    bool isIonJSContext();
    IonJSContext *asIonJSContext();
  }

  IonExclusiveContext *GetIonContext();

  class IonJSContext : public IonExclusiveContext {
    JSContext *cx();
    IonRuntime *ionRuntime();
    IonCompartment *ionCompartment();
  }

  IonJSContext *GetIonContextFromMainThread();

This is probably a separate bug, but it'd be really nice to have that safety in place before moving more things off the main thread.
Where would the constraints be generated, entirely out of the main thread?  Does that imply that we can only have one compilation at a time?  Otherwise I was thinking of Bug 899833 to work around this limitation by having a lifoAlloc for the constraints of each compilation, but we should also add the fact that we want to be able to remove the constraint from the constraint list of TypeSets, such as we can collect only one script while keeping its constraint around.

(In reply to Luke Wagner [:luke] from comment #8)
> Related to this work: do you have any plans to make IonContext safer with
> the new context types?  Given how easy it is to
> GetIonContext()->{cx,runtime,compartment} anywhere from a worker thread,
> this seems like a really big hole in all the great type safety work you've
> been doing recently.

Ion needs to be able to bake in some of these, in case of calls.  But it does not have to use them, we could potentially move all allocations to a Slice, and reuse the separation of the memory that PJS introduced.  In which case we could allocate object such as the template Objects during IonBuilder while running off-main thread.  At the end of the compilation, we can likely GC the slice and move everything to the tenured.
(In reply to Luke Wagner [:luke] from comment #8)
> Related to this work: do you have any plans to make IonContext safer with
> the new context types?  Given how easy it is to
> GetIonContext()->{cx,runtime,compartment} anywhere from a worker thread,
> this seems like a really big hole in all the great type safety work you've
> been doing recently.
> ...
> This is probably a separate bug, but it'd be really nice to have that safety
> in place before moving more things off the main thread.

Agreed.  I don't think that IonContext should expose compartment/runtime pointers at all, and just provide accessors for getting the baked in immutable bits.
(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Where would the constraints be generated, entirely out of the main thread? 
> Does that imply that we can only have one compilation at a time?  Otherwise
> I was thinking of Bug 899833 to work around this limitation by having a
> lifoAlloc for the constraints of each compilation, but we should also add
> the fact that we want to be able to remove the constraint from the
> constraint list of TypeSets, such as we can collect only one script while
> keeping its constraint around.

After bug 906788 and subsequent analyzeTypes-ectomy the only remaining constraints will be those capturing newScript related stuff and freeze constraints for compilation.  The only freeze constraints will be those added on heap type sets, which will be pretty infrequent and it should be fine if the compilation thread just takes a lock when adding them (all threads will need to take the exclusive access lock when allocating from the type lifoalloc).  This would permit any number of compilation threads to be active at once.

> Ion needs to be able to bake in some of these, in case of calls.  But it
> does not have to use them, we could potentially move all allocations to a
> Slice, and reuse the separation of the memory that PJS introduced.  In which
> case we could allocate object such as the template Objects during IonBuilder
> while running off-main thread.  At the end of the compilation, we can likely
> GC the slice and move everything to the tenured.

I'm hoping it won't be necessary to allocate GC things during compilation, it seems like a lot of machinery to preserve some behavior that really just dates back to whatever was easiest when implementing JM.  Alternatives would be to not create template JSObjects but just template JSObject-like-malloced-things, or to have baseline create any necessary template objects when compiling or executing code.
(In reply to Brian Hackett (:bhackett) from comment #11)
> (In reply to Nicolas B. Pierron [:nbp] from comment #9)
> > Ion needs to be able to bake in some of these, in case of calls.  But it
> > does not have to use them, we could potentially move all allocations to a
> > Slice, and reuse the separation of the memory that PJS introduced.  In which
> > case we could allocate object such as the template Objects during IonBuilder
> > while running off-main thread.  At the end of the compilation, we can likely
> > GC the slice and move everything to the tenured.
> 
> I'm hoping it won't be necessary to allocate GC things during compilation,
> it seems like a lot of machinery to preserve some behavior that really just
> dates back to whatever was easiest when implementing JM.  Alternatives would
> be to not create template JSObjects but just template
> JSObject-like-malloced-things, or to have baseline create any necessary
> template objects when compiling or executing code.

I think it would be nicer to be able to allocate some GC things such as JSObject & JSString in some cases.  Otherwise we would have to create these hunk, for every kind of objects and patch the code once we reach the main thread.  This is doable and probably simpler than using the machinery of the GC, on the other hand this is yet another system which has to be created for every GC thing that we might have to allocate.
Depends on: 915473
Depends on: 916914
Depends on: 917441
Depends on: 917590
Depends on: 917952
Depends on: 918116
Depends on: 918161
OS: Mac OS X → All
Hardware: x86 → All
Depends on: 920689
Depends on: 921902
Depends on: 922134
Depends on: 922168
Depends on: 922270
Depends on: 922283
Depends on: 923693
Depends on: 924611
Depends on: 925962
Depends on: 928562
Depends on: 928776
Depends on: 930048
Depends on: 931984
Depends on: 934500
Depends on: 934526
Depends on: 935027
Depends on: 935032
Depends on: 935324
Depends on: 936501
Depends on: 937674
Depends on: 938124
Depends on: 938950
Depends on: 940852
Blocks: 941818
This is now (finally) in reach.  Attached is a rolled up patch that is pretty stable testing locally --- passes jit-tests repeatedly with --ion-eager --ion-parallel-compile=on.  SS seems to improve by a couple % and octane/kraken are more or less flat (these should improve with tuning as we trigger Ion compiles more aggressively, though).
Attachment #656464 - Attachment is obsolete: true
Attachment #656850 - Attachment is obsolete: true
Attachment #658072 - Attachment is obsolete: true
Attachment #658261 - Attachment is obsolete: true
Attachment #658271 - Attachment is obsolete: true
Add a compilation lock and move IonBuilder off thread, taking the lock in the required places.
Attachment #8337441 - Flags: review?(jdemooij)
Ensure that the stack/heap type sets and flags/properties of type objects are protected by the compilation lock.
Attachment #8337442 - Flags: review?(jdemooij)
Related to the last patch, the clasp/proto of a type object can be mutated during execution in some circumstances.  This adds protection code but is mostly putting these public fields behind an API.
Attachment #8337443 - Flags: review?(jdemooij)
IonBuilder needs to be able to read baseline scripts off of scripts, so this adds code to protect these accesses and ensure the baseline script is fully initialized when it is attached to the script.
Attachment #8337444 - Flags: review?(jdemooij)
Protect the mutable parts of a baseline script that can be read by IonBuilder, mainly the stub chains attached to ICs.
Attachment #8337445 - Flags: review?(jdemooij)
When the arguments optimization fails, script->needsArgsObj() changes during execution.  This patch protects this field, but more importantly modifies IonBuilder so that needsArgsObj() is only read once and we don't get confused if it changes during the build.  I haven't seen any bad behavior resulting from potentially allowing compilation of code with the wrong needsArgsObj() value, but maybe it would be better if we tracked this dependency explicitly with a freeze constraint.
Attachment #8337446 - Flags: review?(jdemooij)
IonBuilder can try to get scripts off of functions so delazification of functions needs to be protected with the compilation lock.
Attachment #8337447 - Flags: review?(jdemooij)
Array buffers can be neutered at any time so protect this with the compilation lock.
Attachment #8337448 - Flags: review?(jdemooij)
The callsite clone table is accessed by IonBuilder so accesses to it need to be protected.
Attachment #8337449 - Flags: review?(jdemooij)
Protect global object slots that are lazily initialized and read by IonBuilder.
Attachment #8337450 - Flags: review?(jdemooij)
Protect the contents of the global's intrinsics holder.
Attachment #8337451 - Flags: review?(jdemooij)
Attached patch fix SPS raceSplinter Review
Fix a race condition when enabling or disabling SPS.  Changing this value in flight can crash compilation threads but active compilations weren't canceled until after the value was changed.
Attachment #8337453 - Flags: review?(jdemooij)
This has nothing to do with off thread building but the different allocation order was causing a test to fail because baseline wasn't reporting OOM on failed stub allocation.
Attachment #8337454 - Flags: review?(jdemooij)
Attachment #8337449 - Flags: review?(jdemooij) → review+
Attachment #8337450 - Flags: review?(jdemooij) → review+
Attachment #8337451 - Flags: review?(jdemooij) → review+
Attachment #8337453 - Flags: review?(jdemooij) → review+
Comment on attachment 8337444 [details] [diff] [review]
protect script->baseline

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

::: js/src/jit/BaselineJIT.cpp
@@ +640,5 @@
>  
>  uint8_t *
>  BaselineScript::nativeCodeForPC(JSScript *script, jsbytecode *pc, PCMappingSlotInfo *slotInfo)
>  {
> +    JS_ASSERT_IF(script->hasBaselineScript(), script->baselineScript() == this);

This is needed because fixupJumpTable ends up calling nativeCodeForPC before we set script->baseline? Makes sense.
Attachment #8337444 - Flags: review?(jdemooij) → review+
Comment on attachment 8337446 [details] [diff] [review]
protect script->needsArgsObj()

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

::: js/src/jit/Ion.cpp
@@ +1640,4 @@
>      if (!info)
>          return AbortReason_Alloc;
>  
> +    BaselineInspector *inspector = alloc->new_<BaselineInspector>(script);

CompileInfo has an OOM check, let's add one here too:

if (!inspector)
    return AbortReason_Alloc;

::: js/src/jsscript.cpp
@@ +2924,5 @@
>      JS_ASSERT(!script->isGenerator());
>  
> +    {
> +        AutoLockForCompilation lock(cx);
> +        script->needsArgsObj_ = true;

If we only read/write needsArgsObj_ on the main thread, why do we need this lock?
Attachment #8337446 - Flags: review?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #28)
> ::: js/src/jsscript.cpp
> @@ +2924,5 @@
> >      JS_ASSERT(!script->isGenerator());
> >  
> > +    {
> > +        AutoLockForCompilation lock(cx);
> > +        script->needsArgsObj_ = true;
> 
> If we only read/write needsArgsObj_ on the main thread, why do we need this
> lock?

IonBuilder reads needsArgsObj when deciding whether to inline scripts.
Attachment #8337448 - Flags: review?(jdemooij) → review+
Attachment #8337447 - Flags: review?(jdemooij) → review+
Comment on attachment 8337446 [details] [diff] [review]
protect script->needsArgsObj()

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

(In reply to Brian Hackett (:bhackett) from comment #29)
> > If we only read/write needsArgsObj_ on the main thread, why do we need this
> > lock?
> IonBuilder reads needsArgsObj when deciding whether to inline scripts.

Ah, I see.

::: js/src/jit/Ion.cpp
@@ +1640,4 @@
>      if (!info)
>          return AbortReason_Alloc;
>  
> +    BaselineInspector *inspector = alloc->new_<BaselineInspector>(script);

OOM check.
Attachment #8337446 - Flags: review+
Attachment #8337443 - Flags: review?(jdemooij) → review+
Comment on attachment 8337442 [details] [diff] [review]
protect type information

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

::: js/src/jsworkers.cpp
@@ +566,5 @@
>          JSObject *newProto = GetClassPrototypePure(&parseTask->scopeChain->global(), key);
>          JS_ASSERT(newProto);
>  
> +        // Note: this is safe to do without requiring the compilation lock, as
> +        // the new type is not yet available to compilation threads.

Thanks for these comments, really useful.
Attachment #8337442 - Flags: review?(jdemooij) → review+
Comment on attachment 8337445 [details] [diff] [review]
protect baseline stubs

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

Looks pretty straight-forward/mechanical.
Attachment #8337445 - Flags: review?(jdemooij) → review+
Comment on attachment 8337454 [details] [diff] [review]
fix some random OOM bug

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

This is fine, but there are a lot more places where we call getStub. Either fix the other ones too or file a follow-up bug. r=me with that.
Attachment #8337454 - Flags: review?(jdemooij) → review+
Comment on attachment 8337441 [details] [diff] [review]
locking core code

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

Looks good; sorry for the delay. Please make sure off-thread IonBuilder is disabled by default, at least until after the next merge. I also think we should only enable this after bug 938124 is in.

::: js/src/jscntxt.h
@@ +1086,5 @@
> +            runtime->compilationLockOwner = PR_GetCurrentThread();
> +#endif
> +        } else {
> +            JS_ASSERT(!runtime->mainThreadHasCompilationLock);
> +            runtime->mainThreadHasCompilationLock = true;

This started to make sense after I saw mainThreadHasCompilationLoc was DebugOnly<bool>. Can we wrap this + the definition of compilationLockOwner / mainThreadHasCompilationLoc in #ifdef DEBUG instead?

(Not that it matters much here, but DebugOnly also uses at least 1 byte in opt builds.)
Attachment #8337441 - Flags: review?(jdemooij) → review+
Attached patch ggc fixesSplinter Review
IonBuilder can access nursery pointers via the prototype pointers of type objects.  These can be moved at any time by nursery collections on the main thread, so the attached patch adds logic to avoid accessing object prototypes which might be nursery pointers during optimization.  (In these cases we usually won't be able to do much optimization anyways since any nursery objects won't have singleton type).
Attachment #8346131 - Flags: review?(jdemooij)
Comment on attachment 8346131 [details] [diff] [review]
ggc fixes

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

r=me with nits addressed.

::: js/src/jit/MIR.cpp
@@ +2951,5 @@
>                      }
>                  }
>              }
>  
> +            if (!obj->tenuredProto())

I thought tenuredZone would return a JSObject* instead of bool, just like for instance Cell::tenuredZone() returns a Zone. Renaming it to hasTenuredProto() would avoid the confusion.

::: js/src/jsinfer.cpp
@@ +2656,5 @@
>  /////////////////////////////////////////////////////////////////////
>  
> +#ifdef DEBUG
> +void
> +TypeObject::assertCanAccessProto()

This function is not called anywhere, can we call it from TypeObject::getProto() or something?
Attachment #8346131 - Flags: review?(jdemooij) → review+
Just so that we don't forget about this: can you make sure that when IonBuilder aborts, we correctly mark the script as uncompilable? Maybe it works fine with your patches, but would be good to check this.
Flags: needinfo?(bhackett1024)
(In reply to Jan de Mooij [:jandem] from comment #37)
> Just so that we don't forget about this: can you make sure that when
> IonBuilder aborts, we correctly mark the script as uncompilable? Maybe it
> works fine with your patches, but would be good to check this.

There is some new logic in AttachFinishedCompilations when there is no finished codegen which should handle this case:

+            if (builder->abortReason() == AbortReason_Disable)
+                SetIonScript(builder->script(), builder->info().executionMode(), ION_DISABLED_SCRIPT);
Flags: needinfo?(bhackett1024)
Depends on: 950923
https://hg.mozilla.org/mozilla-central/rev/dbeea0e93b56
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 951129
Pushed a followup to unbreak --disable-threadsafe builds:

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

Pushing to m-c so that we can restart the fuzzers.
Depends on: 951488
Depends on: 952330
Depends on: 967019
Attached patch BackoutSplinter Review
As we've discussed with Brian, we'll back this out due to concerns about the complexity (locking, possible races) of this approach. Also it did not improve performance, at least on our main benchmarks, so it's hard to justify the extra complexity. If it becomes worthwhile in the future, we can consider adding it back, maybe in a different form.
Attachment #8375762 - Flags: review?(jorendorff)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8375762 [details] [diff] [review]
Backout

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

In Ion.cpp, maybe you can also get rid of the `using mozilla::Maybe;` at the top.
Attachment #8375762 - Flags: review?(jorendorff) → review+
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/8c521a802625

I assume we're going to want to back this out from Aurora as well?
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → WONTFIX
Target Milestone: mozilla29 → ---
Comment on attachment 8375762 [details] [diff] [review]
Backout

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A (backout)
User impact if declined: Threading issues or race conditions could cause incorrect behavior, crashes etc.
Testing completed (on m-c, etc.): On m-c
Risk to taking this patch (and alternatives if risky): Fairly big patch but most of it is mechanical, should be pretty low risk.
String or IDL/UUID changes made by this patch: None.
Attachment #8375762 - Flags: approval-mozilla-aurora?
Attachment #8375762 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This backout seems to have triggered a 22% regression in non-asm.js asmjs-apps-lua_binarytrees-loadtime.

Given that the changesets that causes improvements of similar magnitude earlier seem entirely unrelated (and especially landing the patches that were backed out in the regressing changeset didn't improve anything), the test just seems to vary wildly based on ordering changes or something similar. 22% of a fairly long-running test seems like a lot for that, though.
(In reply to Till Schneidereit [:till] from comment #47)
> This backout seems to have triggered a 22% regression in non-asm.js
> asmjs-apps-lua_binarytrees-loadtime.

This is real and expected. The patch was a big improvement when compiling large asm.js scripts with asm.js is disabled.
Depends on: 972364
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: