Closed Bug 1713400 Opened 2 months ago Closed 2 months ago

Generally use page lengths instead of byte lengths for linear memory

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
91 Branch
Tracking Status
firefox91 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

With large-buffers and memory32 the maximum specifiable page size is 2^16 which overflows a uint32_t when converted to a byte length. With memory64 the maximum page size will be 2^48, which will overflow a uint64_t.

We cannot whole-sale reject these values as an implementation limit as it's valid to specify a huge 'maximum' limit with the expectation that engines will just fail to grow to that limit if requested at runtime.

I've found that most logic for memory limits/objects is cleaner expressed in pages, and this works around the overflow issue.

(I think comment 0 has a couple of typos, talking about "maximum page size", it's really "maximum page count" or "maximum memory size".)

This seems reasonable. There's the lingering matter of perhaps one day allowing different page sizes, but at worst that means that we have to carry the specified page size along with the memory size.

Clearly bounds check limits used by jitted code must continue to be expressed in bytes.

Severity: -- → N/A
Type: task → enhancement
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All

This is a clean-up to add a first-class MemoryDesc object,
replacing the ad-hoc methods of describing a module's memory.

Temporarily, this is just a Limits (in pages) and the byte length converted
versions of Limits. The byte length fields will be dropped later.

asm.js used to rely heavily on mutating the ad-hoc representation on
ModuleEnvironment. This commit adds a ModuleEnvironmentShared::Memory
struct for simulating the old way of mutating ModuleEnvironment.

Assignee: nobody → rhunt
Status: NEW → ASSIGNED

Add accessor methods and tweak code to reduce users
that depend on MemoryDesc exposing raw byte lengths for
the initial and maximum fields.

Additionally, MemoryKind is added to MemoryDesc so we can start
asserting in code that assumes Memory32.

Depends on D116650

This commit updates the memory runtime code to generally
use pages instead of byte lengths. In terms of runtime
structures: limits are in pages, memory object max
size is in pages, memory object current size is in bytes,
and the TlsData bounds check limit is in bytes.

This code is tricky, so I've added a 'wasm::Pages' typed unit
to catch cases of type confusion and make intent clearer. I've
also tried to add asserts, comments, and rename unclear variables.

The array buffer's (shared and non-shared) still track their
length in bytes (this is best as the byte length is needed for
warm code in memory.copy/fill), but now the 'maximum' field is
stored in pages. This allows maximum values that would overflow
byte length's with memory64 to be represented.

The array buffer interface for wasm now generally only uses
pages, and most code internal operates on pages. There are
still a few cases where byte lengths are required and we add
code or assert that converting from pages to byte lengths is
valid.

Depends on D116651

Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/89276eacc8d6
wasm: Add MemoryDesc as analog to TableDesc. r=lth
https://hg.mozilla.org/integration/autoland/rev/e3729fc28c4a
wasm: Reduce dependencies on MemoryDesc byte length's. r=lth
https://hg.mozilla.org/integration/autoland/rev/4bf424b6f46a
wasm: Switch memory runtime code to use pages instead of byte lengths. r=lth
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
You need to log in before you can comment on or make changes to this bug.