Closed Bug 1452982 Opened 2 years ago Closed 2 years ago

Some JSContext/JSRuntime/ZoneGroup cleanup

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(15 files)

31.64 KB, patch
jonco
: review+
Details | Diff | Splinter Review
12.03 KB, patch
jonco
: review+
Details | Diff | Splinter Review
49.86 KB, patch
jonco
: review+
Details | Diff | Splinter Review
22.73 KB, patch
jonco
: review+
Details | Diff | Splinter Review
2.56 KB, patch
jonco
: review+
Details | Diff | Splinter Review
60.95 KB, patch
jonco
: review+
Details | Diff | Splinter Review
15.23 KB, patch
jonco
: review+
Details | Diff | Splinter Review
2.56 KB, patch
jonco
: review+
Details | Diff | Splinter Review
22.86 KB, patch
jonco
: review+
Details | Diff | Splinter Review
73.59 KB, patch
jonco
: review+
Details | Diff | Splinter Review
43.99 KB, patch
jonco
: review+
Details | Diff | Splinter Review
6.10 KB, patch
jonco
: review+
Details | Diff | Splinter Review
8.82 KB, patch
jonco
: review+
Details | Diff | Splinter Review
51.09 KB, patch
jonco
: review+
Details | Diff | Splinter Review
9.37 KB, patch
jonco
: review+
Details | Diff | Splinter Review
No description provided.
We can do this since bug 1449135.

Benefits are: (1) a bit faster (2) in debug builds we assert we're on the runtime's main thread.

This also removes some unused code: JS::isGCEnabled and GCRuntime::canChangeActiveContext.
Attachment #8966601 - Flags: review?(jcoppeard)
The plan is to remove ZoneGroup now that cooperative scheduling is gone.

The debuggerList can just move back to JSRuntime (all places where we iterated over this already looked at all zone groups so this should be equivalent).
Attachment #8966619 - Flags: review?(jcoppeard)
Comment on attachment 8966601 [details] [diff] [review]
Part 1 - Use rt->mainContextFromOwnThread() instead of TlsContext.get()

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

Great, thanks for doing this.
Attachment #8966601 - Flags: review?(jcoppeard) → review+
Attachment #8966619 - Flags: review?(jcoppeard) → review+
Priority: -- → P3
This removes ZoneGroup::nursery() and ZoneGroup::storeBuffer().

These just forwarded to JSRuntime*, but it's an extra indirection and note that we even had some code that still assumed nursery per ZoneGroup by looping over all groups!
Attachment #8966706 - Flags: review?(jcoppeard)
This is mostly just moving code, except:

* I made ionLazyLinkList_ and ionLazyLinkListSize_ use ActiveThreadData instead of HelperThreadLockData. Asserting they're only accessed on the main thread is a stronger invariant. (There are some now-redundant asserts in some methods that I kept.)

* numFinishedBuilders is no longer a public member - I added numFinishedBuildersRef that takes the helper thread lock and numFinishedBuilders to return the value unlocked to document the invariants better.
Attachment #8966872 - Flags: review?(jcoppeard)
Comment on attachment 8966706 [details] [diff] [review]
Part 3 - Remove ZoneGroup GC methods

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

::: js/src/builtin/TypedObject.cpp
@@ +1433,5 @@
>  
>      // Trigger a post barrier when attaching an object outside the nursery to
>      // one that is inside it.
>      if (owner && !IsInsideNursery(this) && IsInsideNursery(owner))
> +        runtimeFromActiveCooperatingThread()->gc.storeBuffer().putWholeCell(this);

There's also a storeBuffer() method on Cell that returns the store buffer for cells that are in the nursery, so in cases like this you could write:

  owner->storeBuffer()->putWholeCell(...)

::: js/src/gc/GC.cpp
@@ -1066,5 @@
>  
>      ZealMode zealMode = ZealMode(zeal);
> -    if (zealMode == ZealMode::GenerationalGC) {
> -        for (ZoneGroupsIter group(rt); !group.done(); group.next())
> -            group->nursery().enterZealMode();

Thanks for fixing that :)
Attachment #8966706 - Flags: review?(jcoppeard) → review+
Attachment #8966877 - Flags: review?(jcoppeard) → review+
Attachment #8966872 - Flags: review?(jcoppeard) → review+
This removes ZoneGroup. The remaining fields were moved into GCRuntime or Zone.

I'll remove the almost-empty ZoneGroup.{h,cpp} files separately to avoid weird unified build issues. Also, ZoneGroupData should be renamed to ZoneData.
Attachment #8966940 - Flags: review?(jcoppeard)
Attachment #8966940 - Flags: review?(jcoppeard) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/473b7d381544
part 1 - Use rt->mainContextFromOwnThread() instead of TlsContext.get() in some places. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae45e56c3c71
part 2 - Move debuggerList from ZoneGroup to JSRuntime. r=jonco
Keywords: leave-open
Attachment #8967313 - Flags: review?(jcoppeard)
Oh that latest patch does not rename some related things like ZoneGroupOrGCTaskData, I'll rename these separately.
Attachment #8967316 - Flags: review?(jcoppeard)
I searched for ZoneGroup and "zone group" in js/src and fixed them.
Attachment #8967320 - Flags: review?(jcoppeard)
I'll stop here for now to avoid bitrot, but after this lands there will be a few more renamings (ActiveThread -> MainThread etc) and some comment fixes.
Attachment #8967313 - Flags: review?(jcoppeard) → review+
Attachment #8967316 - Flags: review?(jcoppeard) → review+
Attachment #8967320 - Flags: review?(jcoppeard) → review+
Attachment #8967322 - Flags: review?(jcoppeard) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8104a9c57c8
part 3 - Remove ZoneGroup nursery/storeBuffer methods. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/17debfcdc775
part 4 - Move jit-related fields from ZoneGroup to JitRuntime. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/a56736bcd64a
part 5 - Refactor JitRuntime::isOptimizationTrackingEnabled to take a JSRuntime* instead of ZoneGroup*. r=jonco
(In reply to Jon Coppeard (:jonco) from comment #7)
> There's also a storeBuffer() method on Cell that returns the store buffer
> for cells that are in the nursery, so in cases like this you could write:
> 
>   owner->storeBuffer()->putWholeCell(...)

Good idea, I did that in a few places.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d38e3ca7457a
part 7 - Rename ZoneGroupData to ZoneData. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/35d142c5c599
part 8 - Remove ZoneGroup.h/cpp files. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/a434fac58370
part 9 - Remove/rename remaining ZoneGroup references. r=jonco
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16fcaa62dee9
part 10 - Rename *ActiveCooperatingThread to *MainThread. r=jonco
Attachment #8968149 - Flags: review?(jcoppeard)
Attachment #8968149 - Flags: review?(jcoppeard) → review+
Attachment #8968153 - Flags: review?(jcoppeard) → review+
Attachment #8968166 - Flags: review?(jcoppeard)
Comment on attachment 8968166 [details] [diff] [review]
Part 13 - Misc cleanup

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

::: js/src/vm/TraceLogging.cpp
@@ -879,5 @@
>              /*NOTREACHED*/
>          }
>  
> -        if (strstr(options, "EnableActiveThread"))
> -            cooperatingThreadEnabled = true;

Is this going to break any tools?
Attachment #8968166 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #29)
> Is this going to break any tools?

Good question. I don't expect any breakage (TraceLogger isn't used much) but if it does happen we can either fix the tools or support both values.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb5e6101f4a
part 11 - Rename ActiveThread to MainThread. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/d46b75deaae5
part 12 - Clean up ContextKind and CheckThreadLocal. r=jonco
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb369e8ad78
part 13 - Misc cleanup related to cooperative scheduling. r=jonco
THREAD_TYPE_COOPERATING => THREAD_TYPE_MAIN

MAX_ACTIVE_THREAD_SCRIPT_SIZE => MAX_MAIN_THREAD_SCRIPT_SIZE
MAX_ACTIVE_THREAD_LOCALS_AND_ARGS => MAX_MAIN_THREAD_LOCALS_AND_ARGS
Attachment #8968483 - Flags: review?(jcoppeard)
Comment on attachment 8968481 [details] [diff] [review]
Part 14 - Rename "active thread" to "main thread"

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

::: js/src/gc/GCRuntime.h
@@ +935,5 @@
>  #ifdef DEBUG
>      MainThreadData<bool> arenasEmptyAtShutdown;
>  #endif
>  
> +    /* Synchronize GC heap access among GC helper threads and main threads. */

Should be "and the main thread."

::: js/src/jit/JitOptions.h
@@ +20,2 @@
>  static const uint32_t MAX_ACTIVE_THREAD_SCRIPT_SIZE = 2 * 1000;
>  static const uint32_t MAX_ACTIVE_THREAD_LOCALS_AND_ARGS = 256;

I guess these MAX_ACTIVE_THREAD... constants should be renamed too.

::: js/src/vm/HelperThreads.cpp
@@ +1759,5 @@
>          AutoUnlockHelperThreadState unlock(locked);
>          wasm::ExecuteCompileTaskFromHelperThread(task);
>      }
>  
> +    // No main thread should be waiting on the CONSUMER mutex.

I originally commented that this should be "The main thread should not", but helper threads can be used by multiple runtimes so maybe this does make sense.

@@ +1802,5 @@
>          task->execute();
>          task->dispatchResolveAndDestroy();
>      }
>  
> +    // No main thread should be waiting on the CONSUMER mutex.

Ditto above.
Attachment #8968481 - Flags: review?(jcoppeard) → review+
Comment on attachment 8968483 [details] [diff] [review]
Part 15 - Rename some constants

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

::: js/src/jit/JitOptions.h
@@ +17,5 @@
>  
>  // Longer scripts can only be compiled off thread, as these compilations
>  // can be expensive and stall the main thread for too long.
> +static const uint32_t MAX_MAIN_THREAD_SCRIPT_SIZE = 2 * 1000;
> +static const uint32_t MAX_MAIN_THREAD_LOCALS_AND_ARGS = 256;

Oh, you fixed those here, please ignore my previous comment.
Attachment #8968483 - Flags: review?(jcoppeard) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7d0134b221
part 14 - Rename 'active thread' to 'main thread'. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/292f8e5c6336
part 15 - Rename some constants. r=jonco
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/2f7d0134b221
https://hg.mozilla.org/mozilla-central/rev/292f8e5c6336
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1456494
You need to log in before you can comment on or make changes to this bug.