Closed Bug 1518210 Opened 11 months ago Closed 3 months ago

Make WASM_HUGE_MEMORY a run-time switch

Categories

(Core :: Javascript: WebAssembly, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: lth, Assigned: rhunt)

References

Details

Attachments

(9 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

We currently define WASM_HUGE_MEMORY on all 64-bit platforms and as a result always reserve a 6GB area of virtual address space per wasm Memory to avoid explicit memory bounds checks, this is a significant performance win. (Sometimes we could reserve something closer to 4GB, see eg bug 1442544 for ARM64.)

The large reservation is a problem in some circumstances:

  • On Android + ARM64 the kernel has "only" 4TB address space available, meaning
    we exhaust the available address space with just over 80 live memories per
    process (and we can't hope to have complete process separation on Android).
    If we believe in the success of wasm as a component technology we should
    expect this to become a problem. (4TB = 39 bits of address space.)

  • On Linux + ARM64 with 16KB pages we risk that there's much less virtual
    memory available, at least default kernel configurations appear to allow
    for only 36 bits of address space in this case.

  • On some x64 systems (probably shared systems preconfigured by conservative
    sysadmins) the amount of virtual memory per process has been observed to be
    very low, on the order of 8-16GB. In these situations wasm will be able to
    allocate one or two memories and then fail.

We also don't yet know what the situation is for Windows on ARM64 but there's a risk that the situation is similar to that on Android; the small address space is not a prank but is a result of the TLB design, a larger address space requires a deeper TLB, as I understand it.

Anyway, we need to do two things:

(1) Make it possible to generate explicit bounds checks for 64-bit code
(2) Ideally make it possible to determine at run-time whether to use the
current HUGE_MEMORY strategy or the bounds-check strategy.

With both pieces in place we would then determine at process startup whether the process will use one strategy or the other, depending on what we can find out about available virtual memory. Effectively this turns HUGE_MEMORY into a run-time flag.

Blocks: 1507765

I've got this working now. Here's my current patch queue [1]. Still have some clean-up to do before it's ready for review.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f250fe7fe451d3bae1c609f88510c8511f168479

Assignee: nobody → rhunt

Here's the latest try run [1]. Should be ready for review.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=efb84ea93acff7d3bb5559f38f5a6f1344c12483

x86_64 can re-use MacroAssembler-x86-shared for its wasmBoundsCheck, and so it
doesn't require any new assembler code.

It does require a small baseline compiler change to ensure that TlsData is
loaded if we are going to do a bounds check.

I tested this commit with a x64 try run and manually disabling WASM_HUGE_MEMORY.

The only observed change needed to get bounds checking working on ARM64 was to
implement wasmBoundsCheck in MacroAssembler-arm64.

ARM64 doesn't support predicated instructions like ARM32, so to support spectre
mitigations wasmBoundsCheck emits a 'csel' instruction. I'm not familiar with
how ARM performs speculative execution or how spidermonkey mitigates it, so this
was only a guess.

Depends on D41863

This API is no longer used by IndexDB and can be removed.

Depends on D41864

This commit is the boilerplate for making WASM_HUGE_MEMORY a runtime decision.

Because WasmModule's can be passed across threads with postMessage, we need
to make this decision once per process. The support for this kind of flag seems
ad-hoc, so I stubbed in a global flag in WasmProcess.

A new 'javascript.options.wasm_disable_huge_memory' pref and
'--disable-wasm-huge-memory' JS shell flag are added. These have no effect if
the current platform doesn't support huge memory.

Tests and fuzzing flags are modified to also test with these new flags.

Depends on D41865

To highlight that WASM_HUGE_MEMORY doesn't imply we are using huge memory, this
commit renames the #define.

Most usages of WASM_HUGE_MEMORY are not updated, as they will be removed in
later commits.

Depends on D41866

This commit allows us to conditionally compile bounds checks based on runtime
support for huge memory.

New flags to CompileArgs and CompilerEnvironment are added for whether we are
using huge memory or not, and computed based on the global flag.

Depends on D41867

This commit modifies WasmMemoryObject, ArrayBufferObject,
SharedArrayBufferObject to support conditionally using huge memory based on the
global flag.

The memory logic is fairly involved and entangled, making this change a bit
tricky.

The following changes were made:

  • Stopped conditionally compiling huge memory constants and prefixed them with Huge
  • Stopped conditionally compiling ExtendBufferMapping and wasmMovingGrowToSize
  • Renamed CreateBuffer to CreateSpecificWasmBuffer
    • For clarity
  • Moved maxSize clamping into CreateSpecificWasmBuffer
    • Lets us keep one callsite to wasm::IsHugeMemoryEnabled during memory creation
  • Moved mappedSize computation out of RawbufT::Allocate to CreateSpecificWasmBuffer
    • Lets us keep one callsite to wasm::IsHugeMemoryEnabled during memory creation
  • Moved boundsCheckLimit computation from ArrayBufferObjectMaybeShared to WasmMemoryObject
    • Lets WasmMemoryObject be responsible for knowing whether it is 'huge' or not
  • Added method to determine if a WasmMemoryObject is huge or not
    • Lets use know whether we support movingGrow or have a boundsCheckLimit
  • Refactored WasmMemoryObject::grow to have one callsite to wasmMovingGrowToSize
    • For clarity
  • Added release assert in Module::instantiateMemory
    • Ensures we have a huge memory or bounds checks if needed

Depends on D41868

We can't deserialize code that doesn't contain bounds checks if we have
dynamically switched to not using huge memory. This commit uses BuildID to
invalidate cached code correctly.

Depends on D41869

Memo to self: Is structured clone able to transmit a SAB from a process where we have huge memory to one where we don't, or vice versa? See code in SC algorithm for handling the case where a SAB is transmitted from a process where SABs are allowed to one where they are not.

Blocks: 1507759

This commit extends the jit-test runner to support
'skip-variant-if: $FLAG, $COND', and uses this to not run
'--wasm-disable-huge-memory' tests when the platform doesn't support huge
memory.

Depends on D41870

Going to try to land this soon. Here's a SM only try-run. Working on a Firefox try run now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5126b515716d3f8362c53236ae2aa0f246092b17

Note: I don't intend to land the SystemAddressBits patch. That one needs more testing.

Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/4c8fe76ad293
Wasm: Bounds checking support for x64. r=lth
https://hg.mozilla.org/integration/autoland/rev/12ea41537e05
Wasm: Bounds checking support for ARM64. r=lth
https://hg.mozilla.org/integration/autoland/rev/dc63fd0bbe58
Wasm: Remove unused wasm::DeserializeModule API. r=luke
https://hg.mozilla.org/integration/autoland/rev/eb3fbf8bfb2b
Wasm: Add pref/flag boilerplate for making WASM_HUGE_MEMORY a runtime decision. r=lth
https://hg.mozilla.org/integration/autoland/rev/777aa22c9e8a
Wasm: Rename WASM_HUGE_MEMORY to WASM_SUPPORTS_HUGE_MEMORY. r=lth
https://hg.mozilla.org/integration/autoland/rev/40e3f38af193
Wasm: Conditionally compile bounds checks based on wasm::IsHugeMemoryEnabled. r=lth
https://hg.mozilla.org/integration/autoland/rev/b88d66dddeff
Wasm: Conditionally create huge memory's based on wasm::IsHugeMemoryEnabled. r=lth
https://hg.mozilla.org/integration/autoland/rev/39fc18ada840
Wasm: Make wasm::IsHugeMemoryEnabled() a component of the BuildID for correct invalidation of cached code. r=lth
https://hg.mozilla.org/integration/autoland/rev/6e2e9274465d
Wasm: Don't run --wasm-disable-huge-memory if the platform doesn't support huge memory. r=lth

Backed out 9 changesets (Bug 1518210) for hazard failure on ArrayBufferObject.cpp

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=6e2e9274465d755271a5c1ffc32e1a42a608d879&tochange=ea9924171afd99fe6f60ea4f61ac7a90bad2a0fd&selectedJob=264172546

Backout link: https://hg.mozilla.org/integration/autoland/rev/ea9924171afd99fe6f60ea4f61ac7a90bad2a0fd

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=264172546&repo=autoland&lineNumber=8980

[task 2019-08-30T03:00:45.348Z] Running explain to generate ('hazards.txt', 'unnecessary.txt', 'refs.txt')
[task 2019-08-30T03:00:45.348Z] PATH="/builds/worker/fetches/sixgill/usr/bin:${PATH}" SOURCE='/builds/worker/checkouts/gecko' ANALYZED_OBJDIR='/builds/worker/workspace/obj-analyzed' LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/builds/worker/workspace/obj-haz-shell/dist/bin" XDB='/builds/worker/fetches/sixgill/usr/bin/xdb.so' python2.7 /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/explain.py rootingHazards.txt gcFunctions.txt hazards.txt unnecessary.txt refs.txt
[task 2019-08-30T03:00:45.489Z] Wrote explained_hazards.tmp
[task 2019-08-30T03:00:45.489Z] Wrote unnecessary.tmp
[task 2019-08-30T03:00:45.489Z] Wrote refs.tmp
[task 2019-08-30T03:00:45.489Z] Found 2 hazards 40 unsafe references 0 missing
[task 2019-08-30T03:00:45.490Z] Running heapwrites to generate heapWriteHazards.txt
[task 2019-08-30T03:00:45.490Z] PATH="/builds/worker/fetches/sixgill/usr/bin:${PATH}" SOURCE='/builds/worker/checkouts/gecko' ANALYZED_OBJDIR='/builds/worker/workspace/obj-analyzed' LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/builds/worker/workspace/obj-haz-shell/dist/bin" XDB='/builds/worker/fetches/sixgill/usr/bin/xdb.so' /builds/worker/workspace/obj-haz-shell/dist/bin/js /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/analyzeHeapWrites.js > heapWriteHazards.txt
[task 2019-08-30T03:00:47.789Z] + check_hazards /builds/worker/workspace/analysis
[task 2019-08-30T03:00:47.789Z] + set +e
[task 2019-08-30T03:00:47.789Z] ++ grep -c 'Function.*has unrooted.*live across GC call' /builds/worker/workspace/analysis/rootingHazards.txt
[task 2019-08-30T03:00:47.791Z] + NUM_HAZARDS=2
[task 2019-08-30T03:00:47.791Z] ++ grep -c '^Function.takes unsafe address of unrooted' /builds/worker/workspace/analysis/refs.txt
[task 2019-08-30T03:00:47.792Z] + NUM_UNSAFE=40
[task 2019-08-30T03:00:47.792Z] ++ grep -c '^Function.
has unnecessary root' /builds/worker/workspace/analysis/unnecessary.txt
[task 2019-08-30T03:00:47.794Z] + NUM_UNNECESSARY=1025
[task 2019-08-30T03:00:47.794Z] ++ grep -c '^Dropped CFG' /builds/worker/workspace/analysis/build_xgill.log
[task 2019-08-30T03:00:47.797Z] + NUM_DROPPED=0
[task 2019-08-30T03:00:47.797Z] ++ perl -lne 'print $1 if m!found (\d+)/\d+ allowed errors!' /builds/worker/workspace/analysis/heapWriteHazards.txt
[task 2019-08-30T03:00:47.798Z] + NUM_WRITE_HAZARDS=0
[task 2019-08-30T03:00:47.798Z] ++ grep -c '^Function.expected hazard.but none were found' /builds/worker/workspace/analysis/rootingHazards.txt
[task 2019-08-30T03:00:47.799Z] + NUM_MISSING=0
[task 2019-08-30T03:00:47.799Z] + set +x
[task 2019-08-30T03:00:47.799Z] TinderboxPrint: rooting hazards<br/>2
[task 2019-08-30T03:00:47.799Z] TinderboxPrint: (unsafe references to unrooted GC pointers)<br/>40
[task 2019-08-30T03:00:47.799Z] TinderboxPrint: (unnecessary roots)<br/>1025
[task 2019-08-30T03:00:47.799Z] TinderboxPrint: missing expected hazards<br/>0
[task 2019-08-30T03:00:47.799Z] TinderboxPrint: heap write hazards<br/>0
[task 2019-08-30T03:00:47.800Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'object' of type 'js::SharedArrayBufferObject
' live across GC call at js/src/vm/ArrayBufferObject.cpp:816
[task 2019-08-30T03:00:47.800Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'object' of type 'js::ArrayBufferObject
' live across GC call at js/src/vm/ArrayBufferObject.cpp:816
[task 2019-08-30T03:00:47.801Z] TEST-UNEXPECTED-FAIL | hazards | 2 rooting hazards detected
[task 2019-08-30T03:00:47.801Z] TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit "Inspect Task" link for hazard details
[task 2019-08-30T03:00:47.801Z] + onexit
[task 2019-08-30T03:00:47.801Z] + grab_artifacts /builds/worker/workspace/analysis /builds/worker/artifacts
[task 2019-08-30T03:00:47.801Z] + local analysis_dir
[task 2019-08-30T03:00:47.801Z] + analysis_dir=/builds/worker/workspace/analysis
[task 2019-08-30T03:00:47.801Z] + local artifacts
[task 2019-08-30T03:00:47.801Z] + artifacts=/builds/worker/artifacts
[task 2019-08-30T03:00:47.801Z] + cd /builds/worker/workspace/analysis
[task 2019-08-30T03:00:47.801Z] + ls -lah
[task 2019-08-30T03:00:47.805Z] total 599M

Flags: needinfo?(rhunt)

Well, new task I've learned to perform in try runs..

From a glance, not sure why my patch regressed this. Here's a speculative fix [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b6da0e006d6236f3a8cb17912652f4ad9831734

Flags: needinfo?(rhunt)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/fcc67013b556
Wasm: Bounds checking support for x64. r=lth
https://hg.mozilla.org/integration/autoland/rev/1df22476f295
Wasm: Bounds checking support for ARM64. r=lth
https://hg.mozilla.org/integration/autoland/rev/1e7d2427f348
Wasm: Remove unused wasm::DeserializeModule API. r=luke
https://hg.mozilla.org/integration/autoland/rev/255a6d989d91
Wasm: Add pref/flag boilerplate for making WASM_HUGE_MEMORY a runtime decision. r=lth
https://hg.mozilla.org/integration/autoland/rev/217c5f812296
Wasm: Rename WASM_HUGE_MEMORY to WASM_SUPPORTS_HUGE_MEMORY. r=lth
https://hg.mozilla.org/integration/autoland/rev/47e5adb5ad31
Wasm: Conditionally compile bounds checks based on wasm::IsHugeMemoryEnabled. r=lth
https://hg.mozilla.org/integration/autoland/rev/5bd0f481e985
Wasm: Conditionally create huge memory's based on wasm::IsHugeMemoryEnabled. r=lth
https://hg.mozilla.org/integration/autoland/rev/f493fd84da04
Wasm: Make wasm::IsHugeMemoryEnabled() a component of the BuildID for correct invalidation of cached code. r=lth
https://hg.mozilla.org/integration/autoland/rev/eee3b548e7cd
Wasm: Don't run --wasm-disable-huge-memory if the platform doesn't support huge memory. r=lth
Regressions: 1578031
You need to log in before you can comment on or make changes to this bug.