Revisit SpiderMonkey-related preferences used by workers
Categories
(Core :: JavaScript Engine, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox98 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
(Regressed 1 open bug)
Details
Attachments
(5 files)
We have about:config preferences that affects SpiderMonkey behavior, in javascript.options.*.
There are 2 kind of options that's used by both main thread and workers:
- Prefs reflected to
JS::ContextOptions - Prefs reflected to
JS::RealmCreationOptions
Prefs reflected to JS::ContextOptions are read directly both from main thread and workers
template <typename T>
void SetPrefableContextOptions(JS::ContextOptions& options, T load) {
options
.setAsmJS(load(PREF_NAMES("asmjs")))
...
called from main thread:
static void ReloadPrefsCallback(const char* pref, void* aXpccx) {
...
SetPrefableContextOptions(contextOptions,
[](const char* jsPref, const char* workerPref) {
return Preferences::GetBool(jsPref);
});
and workers:
bool GetWorkerBoolPref(const char* jsPref, const char* workerPref) {
using PrefHelper = PrefTraits<bool>;
if (PrefHelper::Exists(workerPref)) {
return PrefHelper::Get(workerPref);
}
if (PrefHelper::Exists(jsPref)) {
return PrefHelper::Get(jsPref);
}
return PrefHelper::kDefaultValue;
}
...
void LoadContextOptions(const char* aPrefName, void* /* aClosure */) {
...
xpc::SetPrefableContextOptions(contextOptions, GetWorkerBoolPref);
Prefs reflected to JS::RealmCreationOptions are read only from main thread, into Atomic<bool>, and read from workers via the Atomic<bool>.
static mozilla::Atomic<bool> sSharedMemoryEnabled(false);
...
static void ReloadPrefsCallback(const char* pref, void* aXpccx) {
...
sSharedMemoryEnabled =
Preferences::GetBool(JS_OPTIONS_DOT_STR "shared_memory");
void xpc::SetPrefableRealmOptions(JS::RealmOptions& options) {
options.creationOptions()
.setSharedMemoryAndAtomicsEnabled(sSharedMemoryEnabled)
...
Then, prefs directly read from workers have special handling. It allows overriding the main-thread pref value by dom.workers.options.* prefs.
template <typename T>
T GetWorkerPref(const nsACString& aPref,
const T aDefault = PrefTraits<T>::kDefaultValue,
bool* aPresent = nullptr) {
AssertIsOnMainThread();
using PrefHelper = PrefTraits<T>;
T result;
bool present = true;
nsAutoCString prefName;
prefName.AssignLiteral(PREF_WORKERS_OPTIONS_PREFIX);
prefName.Append(aPref);
if (PrefHelper::Exists(prefName.get())) {
result = PrefHelper::Get(prefName.get());
} else {
prefName.AssignLiteral(PREF_JS_OPTIONS_PREFIX);
prefName.Append(aPref);
if (PrefHelper::Exists(prefName.get())) {
result = PrefHelper::Get(prefName.get());...
}
bool GetWorkerBoolPref(const char* jsPref, const char* workerPref) {
using PrefHelper = PrefTraits<bool>;
if (PrefHelper::Exists(workerPref)) {
return PrefHelper::Get(workerPref);
}
if (PrefHelper::Exists(jsPref)) {
return PrefHelper::Get(jsPref);
}
...
There are following issues:
- only some configuration can be overridden for workers, and it's not clear which is which, from the pref name, or the purpose (for example, experimental feature prefs spans across
JS::ContextOptionsandJS::RealmCreationOptions) - there's no actual usage/consumer of the "override" behavior (at least I don't see any)
- there's no description about the "override" behavior around the code
We should revisit this design, and either:
- clearly document the behavior, with some tests
- remove the behavior
|
Assignee | |
Comment 1•2 years ago
|
||
I'll remove the override behavior
|
|
Assignee | |
Comment 2•2 years ago
|
||
|
|
Assignee | |
Comment 3•2 years ago
|
||
Depends on D133975
|
|
Assignee | |
Comment 4•2 years ago
|
||
Also fixed the case of the preference name (gcZeal => gczeal).
Depends on D133976
|
|
Assignee | |
Comment 5•2 years ago
|
||
Depends on D133977
|
|
Assignee | |
Comment 6•2 years ago
|
||
Depends on D133978
|
|
Assignee | |
Updated•2 years ago
|
Pushed by arai_a@mac.com: https://hg.mozilla.org/integration/autoland/rev/cee175fa09e7 Part 1: Remove worker-specific override from JS context options. r=jandem https://hg.mozilla.org/integration/autoland/rev/824c5cd5ea17 Part 2: Remove worker-specific override from JS GC memory options. r=dom-worker-reviewers,smaug https://hg.mozilla.org/integration/autoland/rev/51820e97c1eb Part 3: Remove worker-specific override from JS GC zeal options. r=dom-worker-reviewers,smaug https://hg.mozilla.org/integration/autoland/rev/2f2368b8f931 Part 4: Stop registering callback for worker override. r=jandem https://hg.mozilla.org/integration/autoland/rev/f671659046b1 Part 5: Initialize classStaticBlocks_ field. r=jandem
|
||
Comment 8•2 years ago
|
||
Backed out for causing hybrid bustages on RuntimeService
Backout link : https://hg.mozilla.org/integration/autoland/rev/2c5b36d37c1c886386f0f685e364d0c53f0f0219
Link to failure log : https://treeherder.mozilla.org/logviewer?job_id=363709918&repo=autoland
Failure message:
/builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp:166:30: error: unused variable 'kDefaultValue' [-Werror,-Wunused-const-variable]
|
|
Assignee | |
Updated•2 years ago
|
Pushed by arai_a@mac.com: https://hg.mozilla.org/integration/autoland/rev/fe3db4065f77 Part 1: Remove worker-specific override from JS context options. r=jandem https://hg.mozilla.org/integration/autoland/rev/f9891534f80f Part 2: Remove worker-specific override from JS GC memory options. r=dom-worker-reviewers,smaug https://hg.mozilla.org/integration/autoland/rev/f5d5f154bb33 Part 3: Remove worker-specific override from JS GC zeal options. r=dom-worker-reviewers,smaug https://hg.mozilla.org/integration/autoland/rev/19ac6f4ff625 Part 4: Stop registering callback for worker override. r=jandem https://hg.mozilla.org/integration/autoland/rev/5873688a4ba0 Part 5: Initialize classStaticBlocks_ field. r=jandem
|
||
Comment 10•2 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/fe3db4065f77
https://hg.mozilla.org/mozilla-central/rev/f9891534f80f
https://hg.mozilla.org/mozilla-central/rev/f5d5f154bb33
https://hg.mozilla.org/mozilla-central/rev/19ac6f4ff625
https://hg.mozilla.org/mozilla-central/rev/5873688a4ba0
Description
•