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

Add support for memory growth #3569

Merged
merged 2 commits into from
Jun 26, 2024
Merged

Conversation

bnason-nf
Copy link
Contributor

No description provided.

core/iwasm/common/wasm_memory.c Show resolved Hide resolved
core/iwasm/common/wasm_memory.c Show resolved Hide resolved
core/iwasm/common/wasm_memory.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh wenyongh merged commit 74dbafc into bytecodealliance:main Jun 26, 2024
378 checks passed
@@ -1842,6 +1842,10 @@ WASM_RUNTIME_API_EXTERN bool
wasm_runtime_is_import_global_linked(const char *module_name,
const char *global_name);

WASM_RUNTIME_API_EXTERN bool
wasm_runtime_enlarge_memory(wasm_module_inst_t module_inst,
uint32_t inc_page_count);
Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess it's better to use uint64_t for memory64.

Copy link
Contributor

@wenyongh wenyongh Jun 26, 2024

Choose a reason for hiding this comment

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

In the current design of memory64, the max page count is UINT32_MAX since the max size of linear memory size is 274TB roughly and should be able to meet most of the requirements:

#define DEFAULT_NUM_BYTES_PER_PAGE 65536
#define DEFAULT_MAX_PAGES 65536
#define DEFAULT_MEM64_MAX_PAGES UINT32_MAX
/* Max size of linear memory */
#define MAX_LINEAR_MEMORY_SIZE (4 * (uint64)BH_GB)
/* Roughly 274 TB */
#define MAX_LINEAR_MEM64_MEMORY_SIZE \
(DEFAULT_MEM64_MAX_PAGES * (uint64)64 * (uint64)BH_KB)
/* Macro to check memory flag and return appropriate memory size */
#define GET_MAX_LINEAR_MEMORY_SIZE(is_memory64) \
(is_memory64 ? MAX_LINEAR_MEM64_MEMORY_SIZE : MAX_LINEAR_MEMORY_SIZE)

And the type of page count in WASMMemoryInstance is kept uint32:

uint32 cur_page_count;
/* Maximum page count */
uint32 max_page_count;

So here using uint32 should be also OK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

whatever limitations the current implementation has, it's better to make the user-facing api align with the spec and future-proof, IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, @bnason-nf, could you upload another PR to change uint32_t to uint64_t? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Submitted #3573.

As mentioned by @wenyongh, there are other existing 32 bit memory limitations. Aside from the many uint32 types used internally for memory sizes, including by the direct aot_enlarge_memory() and wasm_enlarge_memory() calls by this new function, there are also many places in the public API that use uint32_t for memory sizes. So as I'm sure everyone is aware, PR #3573 is just a tiny piece of a much larger effort that will need to take place to make WAMR truly 64 bit compatible.

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.

4 participants