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

feat: lowMemoryLimit can accept more than 1 page value #2857

Closed
wants to merge 6 commits into from

Conversation

HerrCai0907
Copy link
Member

@HerrCai0907 HerrCai0907 commented Jun 24, 2024

The original lowMemoryLimit only support memory less than 64kB (1 wasm page).
This PR wants to extend the function of lowMemoryLimit to support more than 64kB memory limitation.

@CountBleck
Copy link
Member

What's the use case for this?

@HerrCai0907
Copy link
Member Author

In high performance embedded device which has larger RAM than the traditional embedded device but not too much (>1M). In this case, 64kB per page is still to much granularity.
Then it can use lowMemoryLimit to control the memory usage.

@CountBleck
Copy link
Member

Does this do anything that --maximumMemory doesn't?

@HerrCai0907
Copy link
Member Author

Does this do anything that --maximumMemory doesn't?

maximumMemory is in 64kB size, but lowMemoryLimit is 16B size. But I agree we may can unify them.

@CountBleck
Copy link
Member

Hmm, I think I'm confused. If you have more than 64 KiB of memory on your device, wouldn't --maximumMemory work?

Or are you restricted to an amount of memory that's not a multiple of 64 KiB (for instance, 68 KiB, 123 KiB, etc.)?

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented Jun 25, 2024

Or are you restricted to an amount of memory that's not a multiple of 64 KiB (for instance, 68 KiB, 123 KiB, etc.)?

Yes. For example, I have device with 150kB memory but want to run 2 wasm job. It is impossible to split 3 x 64kB linear memory.
By this option, each job can use 75kB.

Some background:
Lots of runtime use virtual memory which will not commit the linear memory from memory.grow utill it is really used. So the memory.grow will / can success even we don't have so many physical memory.
When WASM want to write some data in linear memory, it will commit it and make it writable.
But AS's allocator will write information at the end of linear memory, which will make runtime commit the whole linear memory. Then when runtime wants to commit the memory, it will trigger OOM and be killed.

Copy link
Member

@CountBleck CountBleck left a comment

Choose a reason for hiding this comment

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

I don't know the intricacies of TLSF and --lowMemoryLimit. @dcodeIO is better suited to handle this.

std/assembly/rt/tlsf.ts Show resolved Hide resolved
std/assembly/rt/tlsf.ts Show resolved Hide resolved
std/assembly/rt/tlsf.ts Show resolved Hide resolved
cli/options.json Outdated Show resolved Hide resolved
@CountBleck CountBleck requested a review from dcodeIO June 25, 2024 05:05
Comment on lines 381 to +385
let end = <usize>endU64;
if (ASC_LOW_MEMORY_LIMIT) {
end = <u64>ASC_LOW_MEMORY_LIMIT;
if (start > ASC_LOW_MEMORY_LIMIT) unreachable();
}
Copy link
Member

Choose a reason for hiding this comment

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

@HerrCai0907 would this work? Also, what's the background behind overwriting endU64?

Suggested change
let end = <usize>endU64;
if (ASC_LOW_MEMORY_LIMIT) {
end = <u64>ASC_LOW_MEMORY_LIMIT;
if (start > ASC_LOW_MEMORY_LIMIT) unreachable();
}
if (ASC_LOW_MEMORY_LIMIT) endU64 = ASC_LOW_MEMORY_LIMIT;
let end = <usize>endU64;

Copy link
Member Author

Choose a reason for hiding this comment

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

The original implement is u32, but huge memory will cause u32 overflow and failed the later assert.
I don't want to mix the debug assert (it should not happen) and oom (it may happen).

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how that overflow could happen. Memories larger than 4 GiB only exist when using memory64, and in that case, usize would be equivalent to u64, not u32 (so no overflow could take place).

Also, about separating those assertions, you can just change the if (DEBUG) on the next line to if (ASC_LOW_MEMORY_LIMIT || DEBUG).

One last thing: can't ASC_LOW_MEMORY_LIMIT be larger than the current memory size in bytes? An instance could start with a memory of 2 pages, but ASC_LOW_MEMORY_LIMIT could be set to 224 KiB (3 and a half pages). In initialize and computeSize, addMemory is called with an endU64 argument of memory.size() << 16, which is the number of bytes available to the instance (if we assume every page has been committed). However, with this change, the value of end is changed in addMemory from the given value (memory.size() << 16) to ASC_LOW_MEMORY_LIMIT. While this wouldn't have been a problem before, since previously this was only supposed to work with amounts smaller than 1 page, ASC_LOW_MEMORY_LIMIT could be larger than memory.size() << 16, and that could lead to addMemory adding memory that the instance doesn't have yet (and in turn, cause a crash due to an OOB memory access when someone tries to allocate there).

Copy link
Member Author

Choose a reason for hiding this comment

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

the key point is 4gb, it will overflow to zero

Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

@github-actions github-actions bot added the stale label Aug 25, 2024
@HerrCai0907 HerrCai0907 deleted the support-max-szie branch September 26, 2024 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants