Closed Bug 1575153 Opened 1 year ago Closed 5 months ago

Remove AutoFlushICache

Categories

(Core :: JavaScript Engine: JIT, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

There's a lot of complexity associated with AutoFlushICache: it supports nesting (even though we never do that), it does TLS/JSContext lookups, there's the setRange/flush no-op thing, etc.

I think we should simplify as follows:

  • Do icache flushing in ExecutableAllocator::makeExecutable (maybe rename to makeExecutableAndFlushICache). This is used by AutoWritableJitCode.
  • We don't do a lot of patching anymore but for the few places where this might be too much overhead we could add an opt-out, custom flush range or something. I think that only applies to moving GC tracing and Ion invalidation.

(In reply to Jan de Mooij [:jandem] from comment #0)

  • Do icache flushing in ExecutableAllocator::makeExecutable (maybe rename to makeExecutableAndFlushICache). This is used by AutoWritableJitCode.

What's also nice about this is that Wasm code already has this cacheFlush + makeExecutable pattern so we could convert to use the new combined method.

Depends on: 1575161
Depends on: 1575188
Priority: -- → P1
Depends on: 1575470

Flushing on mprotect is a lot simpler and it's easier to reason about
correctness. The next patch will remove the AutoFlushICache infrastructure.

Because mprotect is pretty expensive we've been moving away from code patching
in perf-sensitive cases so I don't expect any serious regressions from this.
(This patch could be optimized more, if needed, but it'd add more complexity.)

Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90aeb73dadaf
part 1 - Flush ICache when making code executable. r=tcampbell,lth
https://hg.mozilla.org/integration/autoland/rev/f5bc71d6ee11
part 2 - Remove AutoFlushICache infrastructure. r=tcampbell,lth
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.