IonMonkey: off thread MIR construction

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
6 years ago
3 months ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 wontfix, firefox30 wontfix)

Details

(Whiteboard: [ion:t])

Attachments

(16 attachments, 5 obsolete attachments)

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
(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 656464 [details] [diff] [review]
part 1 (7566291ca483)

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)
(Assignee)

Comment 2

6 years ago
Created attachment 656850 [details] [diff] [review]
GetPropertyCache fix

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 3

6 years ago
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+

Updated

6 years ago
Attachment #656850 - Flags: review?(luke) → review+
Whiteboard: [ion:t]
(Assignee)

Comment 4

6 years ago
Created attachment 658072 [details] [diff] [review]
part 2 (dd9554c236dc + other pending parallel compilation patches)

> 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)
(Assignee)

Comment 5

6 years ago
Created attachment 658261 [details] [diff] [review]
part 3

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)
(Assignee)

Comment 6

6 years ago
Created attachment 658271 [details] [diff] [review]
patch bomb (dd9554c236dc)

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

Updated

5 years ago
Attachment #658072 - Flags: review?(dvander)
(Assignee)

Updated

5 years ago
Attachment #658261 - Flags: review?(dvander)
(Assignee)

Comment 7

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

Updated

5 years ago
Depends on: 906788

Comment 8

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

Comment 10

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

Comment 11

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

Updated

5 years ago
Depends on: 915473
(Assignee)

Updated

5 years ago
Depends on: 916914
(Assignee)

Updated

5 years ago
Depends on: 917441
(Assignee)

Updated

5 years ago
Depends on: 917590
(Assignee)

Updated

5 years ago
Depends on: 917952
(Assignee)

Updated

5 years ago
Depends on: 918116
(Assignee)

Updated

5 years ago
Depends on: 918161
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

5 years ago
Depends on: 920689
(Assignee)

Updated

5 years ago
Depends on: 921902
(Assignee)

Updated

5 years ago
Depends on: 922134
(Assignee)

Updated

5 years ago
Depends on: 922168
(Assignee)

Updated

5 years ago
Depends on: 922270
(Assignee)

Updated

5 years ago
Depends on: 922283
(Assignee)

Updated

4 years ago
Depends on: 923693
(Assignee)

Updated

4 years ago
Depends on: 924611
(Assignee)

Updated

4 years ago
Depends on: 925962
(Assignee)

Updated

4 years ago
Depends on: 928562
(Assignee)

Updated

4 years ago
Depends on: 928776
(Assignee)

Updated

4 years ago
Depends on: 930048
(Assignee)

Updated

4 years ago
Depends on: 931984
(Assignee)

Updated

4 years ago
Depends on: 934500
(Assignee)

Updated

4 years ago
Depends on: 934526
(Assignee)

Updated

4 years ago
Depends on: 935027
(Assignee)

Updated

4 years ago
Depends on: 935032
(Assignee)

Updated

4 years ago
Depends on: 935324
(Assignee)

Updated

4 years ago
Depends on: 936501
(Assignee)

Updated

4 years ago
Depends on: 937674
(Assignee)

Updated

4 years ago
Depends on: 938124
(Assignee)

Updated

4 years ago
Depends on: 938950
(Assignee)

Updated

4 years ago
Depends on: 940852
Blocks: 941818
(Assignee)

Comment 13

4 years ago
Created attachment 8337433 [details] [diff] [review]
rolled up patch (d4662cf71175)

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
(Assignee)

Comment 14

4 years ago
Created attachment 8337441 [details] [diff] [review]
locking core code

Add a compilation lock and move IonBuilder off thread, taking the lock in the required places.
Attachment #8337441 - Flags: review?(jdemooij)
(Assignee)

Comment 15

4 years ago
Created attachment 8337442 [details] [diff] [review]
protect type information

Ensure that the stack/heap type sets and flags/properties of type objects are protected by the compilation lock.
Attachment #8337442 - Flags: review?(jdemooij)
(Assignee)

Comment 16

4 years ago
Created attachment 8337443 [details] [diff] [review]
protect type clasp/proto

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)
(Assignee)

Comment 17

4 years ago
Created attachment 8337444 [details] [diff] [review]
protect script->baseline

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)
(Assignee)

Comment 18

4 years ago
Created attachment 8337445 [details] [diff] [review]
protect baseline stubs

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)
(Assignee)

Comment 19

4 years ago
Created attachment 8337446 [details] [diff] [review]
protect script->needsArgsObj()

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)
(Assignee)

Comment 20

4 years ago
Created attachment 8337447 [details] [diff] [review]
protect against delazification

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)
(Assignee)

Comment 21

4 years ago
Created attachment 8337448 [details] [diff] [review]
protect against array buffer neutering

Array buffers can be neutered at any time so protect this with the compilation lock.
Attachment #8337448 - Flags: review?(jdemooij)
(Assignee)

Comment 22

4 years ago
Created attachment 8337449 [details] [diff] [review]
protect call site clone table

The callsite clone table is accessed by IonBuilder so accesses to it need to be protected.
Attachment #8337449 - Flags: review?(jdemooij)
(Assignee)

Comment 23

4 years ago
Created attachment 8337450 [details] [diff] [review]
protect standard classes

Protect global object slots that are lazily initialized and read by IonBuilder.
Attachment #8337450 - Flags: review?(jdemooij)
(Assignee)

Comment 24

4 years ago
Created attachment 8337451 [details] [diff] [review]
protect intrinsics

Protect the contents of the global's intrinsics holder.
Attachment #8337451 - Flags: review?(jdemooij)
(Assignee)

Comment 25

4 years ago
Created attachment 8337453 [details] [diff] [review]
fix SPS race

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)
(Assignee)

Comment 26

4 years ago
Created attachment 8337454 [details] [diff] [review]
fix some random OOM bug

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)

Updated

4 years ago
Attachment #8337449 - Flags: review?(jdemooij) → review+

Updated

4 years ago
Attachment #8337450 - Flags: review?(jdemooij) → review+

Updated

4 years ago
Attachment #8337451 - Flags: review?(jdemooij) → review+

Updated

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

Comment 29

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

Updated

4 years ago
Attachment #8337448 - Flags: review?(jdemooij) → review+

Updated

4 years ago
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+

Updated

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

Comment 35

4 years ago
Created attachment 8346131 [details] [diff] [review]
ggc fixes

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)
(Assignee)

Comment 38

4 years ago
(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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
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: 951285
Depends on: 951488
Depends on: 952022
(Assignee)

Updated

4 years ago
Depends on: 952330
Depends on: 967019
Depends on: 969778
Created attachment 8375762 [details] [diff] [review]
Backout

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)

Updated

4 years ago
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
Last Resolved: 4 years ago4 years ago
status-firefox29: --- → fixed
status-firefox30: --- → wontfix
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.
https://hg.mozilla.org/releases/mozilla-aurora/rev/9e00f4eef081

(Forgot to add a=sledru.)
status-firefox29: fixed → wontfix
Depends on: 972364
You need to log in before you can comment on or make changes to this bug.