Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

experiment: lazy migration for stable memory for regions #4171

Merged
merged 18 commits into from
Aug 16, 2023

Conversation

crusso
Copy link
Contributor

@crusso crusso commented Aug 13, 2023

builds on the unwieldly #3768.

Introduces lazy migration of stable memory format 0/1 (no ESM, some ESM) to 2 (Regions) by

  1. Adding an explicit global (StableMem.get/set_version) that tracks the current version.
  2. Transitioning from 0 to 1 on first (acutally growing) StableMem.grow.
  3. Transitioning from 0/1 to 2 on first Region.new().
  4. Testing current version on each (legacy) StableMemory operation and dispatching to the correct legacy/region implementation.

As a result programs have zero overhead until they allocate the first region and all programs can access Region.mo without having to set a flag.

In addition, ExperimentalStableMemory retains the same low space overhead as before, at least until the first region is allocated. Each ESM read-write is slightly slower since it has to switch on the version, but I can live with that.

Avoids:

  • the need to set a flag to enable Regions (and the need to explain and document that)
  • significant overhead of region metadata (1-8MB) until regions are actually used.
  • language dependence on compiler flag (c.f. Haskell).

Removes the need for a user facing --stable-regions flag. Instead, for testing, the flag now just forces eager
(not lazy) migration to version 2.

Drive-by: remove prim stableMemoryRegion since its dangerous (the region returned won't alias the internal region0 after an upgrade without more work).

@crusso crusso changed the base branch from master to multiple-stable-memory August 13, 2023 16:59
@github-actions
Copy link

github-actions bot commented Aug 14, 2023

Comparing from ee79edf to 36a5eda:
In terms of gas, 1 tests regressed, 2 tests improved and the mean change is +6.6%.
In terms of size, 5 tests improved and the mean change is -0.2%.

@crusso crusso marked this pull request as ready for review August 15, 2023 06:48
@crusso crusso requested a review from matthewhammer August 15, 2023 06:48
Copy link
Contributor

@matthewhammer matthewhammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo the StableMem and StableMemory distinction in compile.ml. Maybe we can combine and consolidate these in the future, or explain why they are separate?

@matthewhammer
Copy link
Contributor

Each ESM read-write is slightly slower since it has to switch on the version, but I can live with that.

Okay. Sounds good.

Removes the need for a user facing --stable-regions flag. Instead, for testing, the flag now just forces eager
(not lazy) migration to version 2.

Also sounds good.

@crusso
Copy link
Contributor Author

crusso commented Aug 15, 2023

@matthewhammer thanks for taking a look!

@crusso crusso requested a review from luc-blaeser August 16, 2023 12:06
@@ -532,8 +532,17 @@ module E = struct
| ts -> VarBlockType (nr (func_type env (FuncType ([], ts))))

let if_ env tys thn els = G.if_ (as_block_type env tys) thn els

(* NB: confuses wasm-opt, don't use for now
let _multi_if_ env tys1 tys2 thn els =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity: Is wasm-opt failing or creating invalid code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully recall, but I think it was failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 280ed54 produces this error. Basically I was trying to conditionally call one of two functions with args already on the stack and wasm-opt barfed:

test-bench> @@ -0,0 +1,4 @@
test-bench> +Error: Failed to read module
test-bench> +
test-bench> +Caused by:
test-bench> +    [parse exception: Block requires more values than are available (at 0:57416)]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably be a good citizen and report the bug, but I'd rather make progress..

Copy link
Contributor

@luc-blaeser luc-blaeser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great simplification with lazy region metadata allocation.
Also, very nice refactoring regarding region0.

…really); change assert in get_region0 to debug assert for perf
@crusso crusso merged commit da0e23f into multiple-stable-memory Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants