Closed Bug 1732168 Opened 3 years ago Closed 3 years ago

Use bytecode pinning with ScriptPreloader

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox95 --- fixed

People

(Reporter: tcampbell, Assigned: arai)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Try to use the CompileOptions::usePinnedBytecode option for ScriptPreloader since that mmap file is alive for as long as the runtime. This should reduce Base Content JS overhead.

Blocks: 1523749
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/a75f9be5cd6e
Use bytecode pinning with ScriptPreloader. r=tcampbell
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch

Backed out for causing talos crashes @ js::InternalCallOrConstruct

Backout link

Push with failures

Failure log

Status: RESOLVED → REOPENED
Flags: needinfo?(arai.unmht)
Resolution: FIXED → ---
Target Milestone: 94 Branch → ---

This accidentally enabled bytecode pinning also for StartupCache, because the same option is used for both StartupCache and ScriptPreloader when reading.
StartupCache has cache invalidation, and we cannot use bytecode pinning.
we should enable bytecode pinning only on ScriptPreloader cache.

also there's lifetime issue around shutdown, that ccov hit
we need to make sure the buffer is freed only after the JS is fully shutdown

Regressions: 1732406

ScriptPreloader singleton is freed on ShutdownPhase::XPCOMShutdownFinal phase,
and the mmap-ed file is munmap-ed there.

https://searchfox.org/mozilla-central/rev/a2118a17fe5e4f63d9f011db3111beb995b96a91/js/xpconnect/loader/ScriptPreloader.cpp#108

ScriptPreloader& ScriptPreloader::GetSingleton() {
  static RefPtr<ScriptPreloader> singleton;

  if (!singleton) {
    if (XRE_IsParentProcess()) {
      singleton = new ScriptPreloader();
      singleton->mChildCache = &GetChildSingleton();
      Unused << singleton->InitCache();
    } else {
      singleton = &GetChildSingleton();
    }

    ClearOnShutdown(&singleton);
  }

  return *singleton;
}

ScriptPreloader& ScriptPreloader::GetChildSingleton() {
  static RefPtr<ScriptPreloader> singleton;

  if (!singleton) {
    singleton = new ScriptPreloader();
    if (XRE_IsParentProcess()) {
      Unused << singleton->InitCache(u"scriptCache-child"_ns);
    }
    ClearOnShutdown(&singleton);
  }

  return *singleton;
}

https://searchfox.org/mozilla-central/rev/a2118a17fe5e4f63d9f011db3111beb995b96a91/xpcom/base/ClearOnShutdown.h#110-119

template <class SmartPtr>
inline void ClearOnShutdown(
    SmartPtr* aPtr, ShutdownPhase aPhase = ShutdownPhase::XPCOMShutdownFinal) {
  using namespace ClearOnShutdown_Internal;

  MOZ_ASSERT(NS_IsMainThread());
  MOZ_ASSERT(aPhase != ShutdownPhase::ShutdownPhase_Length);

  InsertIntoShutdownList(new PointerClearer<SmartPtr>(aPtr), aPhase);
}

that happens before JS_ShutDown.

https://searchfox.org/mozilla-central/rev/a2118a17fe5e4f63d9f011db3111beb995b96a91/xpcom/build/XPCOMInit.cpp#650-734

nsresult ShutdownXPCOM(nsIServiceManager* aServMgr) {
...
  // Notify observers of xpcom shutting down
  {
...
    mozilla::KillClearOnShutdown(ShutdownPhase::XPCOMShutdownFinal);
...
  }
...
  nsCycleCollector_shutdown(shutdownCollect);
...
  if (sInitializedJS) {
    // Shut down the JS engine.
    JS_ShutDown();
    sInitializedJS = false;
  }

ccov's failure happens in GC in nsCycleCollector_shutdown.

ScriptPreloader must be live longer than JS_ShutDown, that means we need to have ShutdownPhase after that.

Just moving the ScriptPreloader shutdown after JS_ShutDown causes another crash around SharedImmutableStringsCache
https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=78d72c7a97bc8ceb661b7c65e1cc403de6847958&selectedTaskRun=DIiwUXxpReG5GkprStm8lg.0

there seems to be another dependency

Depends on D126632

mmap-ed file needs to be live longer than JS runtime, and it's different than
ScriptPreloader's lifetime.

Depends on D126633

Attachment #9242643 - Attachment description: Bug 1732168 - Use bytecode pinning with ScriptPreloader. r?tcampbell! → Bug 1732168 - Part 4: Use bytecode pinning with ScriptPreloader. r?tcampbell!
Flags: needinfo?(arai.unmht)
Regressions: 1732411
Regressions: 1732400
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/12ed363393af
Part 1: Remove unused ShutdownPhase::Last variant. r=kmag
https://hg.mozilla.org/integration/autoland/rev/1fcebc384393
Part 2: Add ShutdownPhase after JS_ShutDown. r=kmag
https://hg.mozilla.org/integration/autoland/rev/8b2fb3f5ed01
Part 3: Move AutoMemMap out of ScriptPreloader. r=kmag
https://hg.mozilla.org/integration/autoland/rev/8bbe0638c3b7
Part 4: Use bytecode pinning with ScriptPreloader. r=tcampbell
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: