Skip to content

fix: add BLOCK_OVERHEAD before round size #2652

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

Merged
merged 6 commits into from
Jul 18, 2023

Conversation

HerrCai0907
Copy link
Member

@HerrCai0907 HerrCai0907 commented Feb 15, 2023

When calling heap.alloc(1), It will alloc a block size = BLOCK_MINSIZE (12bytes)
And then
invRound = 27
clz(size) = 28
(1 << (invRound - clz(size))) - 1 = 2147483647

Runtime will grow lots of memory.

@HerrCai0907 HerrCai0907 requested a review from dcodeIO February 15, 2023 10:15
@dcodeIO
Copy link
Member

dcodeIO commented Feb 15, 2023

I might be missing something, but with invRound = 28 and clz(size) = 27, isn't (1 << (invRound - clz(size))) - 1 = 1?

  (1 << (invRound - clz(size))) - 1
= (1 << (28 - 27)) - 1
= (1 << 1) - 1
= 2 - 1 = 1

@HerrCai0907
Copy link
Member Author

I might be missing something, but with invRound = 28 and clz(size) = 27, isn't (1 << (invRound - clz(size))) - 1 = 1?

1 << -1
1 << (-1 mod 32)
1 << 31

@MaxGraey
Copy link
Member

MaxGraey commented Feb 15, 2023

Good catch! 1 << -1 is really equal to 2147483648 (if it's unsigned)

@dcodeIO
Copy link
Member

dcodeIO commented Feb 15, 2023

Still not seeing how this becomes -1 given 28 - 27. What am I missing?

@MaxGraey
Copy link
Member

Hmm, it should be possible when invRound < clz(size) for example when invRound = 27, clz(size) = 28.
@HerrCai0907 Perhaps you misspelled the initial message?

@MaxGraey
Copy link
Member

MaxGraey commented Feb 15, 2023

  • size is less than 536870910 (BLOCK_MAXSIZE >> 1) so clz(size) equal from 3 (when size = 536870910) to 31 (when size = 0). Or size has lower bound 15? In this case clz(15) = 28
  • invRound is always 27

@HerrCai0907
Copy link
Member Author

Hmm, it should be possible when invRound < clz(size) for example when invRound = 27, clz(size) = 28. @HerrCai0907 Perhaps you misspelled the initial message?

Yes, you are right. I have edited the message.

@MaxGraey
Copy link
Member

However, I can't reproduce this problem in playground

What am I doing wrong?

@dcodeIO
Copy link
Member

dcodeIO commented Feb 15, 2023

Thanks for the clarification. So, from locking at the code, growMemory is only called in one place:

/** Allocates a block of the specified size. */
export function allocateBlock(root: Root, size: usize): Block {
let payloadSize = prepareSize(size);
let block = searchBlock(root, payloadSize);
if (!block) {
growMemory(root, payloadSize);

There, the size passed to growMemory has always been prepared to be a valid block size that is not smaller than BLOCK_MINSIZE. Doesn't that prevent the issue from happening?

@MaxGraey
Copy link
Member

MaxGraey commented Feb 15, 2023

I think for reproduction of this we should have initialMemory = 0 which is by default always 1. That's why we've never encountered this problem

@dcodeIO
Copy link
Member

dcodeIO commented Feb 15, 2023

Doesn't seem to me that the issue depends on the current size of memory. At least the computation in question doesn't use it (neither invRound nor size depend on it), instead is only used after to compute the delta in pages.

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented Feb 15, 2023

It is easy to re-produce it.

These code will trace memory.size is 32770 at last instead of 2.

for (let i=0;i<6000;i++) {
  heap.alloc(1);
  trace("", 1, memory.size());
}

I found some unusual memory consumption behavior from the monitor and find the root cause.
It only can be able to discover when running wasm on some memory-use-sensitive device.

@HerrCai0907
Copy link
Member Author

There, the size passed to growMemory has always been prepared to be a valid block size that is not smaller than BLOCK_MINSIZE. Doesn't that prevent the issue from happening?

BLOCK_MINSIZE is 12 instead of 16 @dcodeIO

@dcodeIO
Copy link
Member

dcodeIO commented Apr 25, 2023

Is the underlying problem perhaps that searchBlock does not round when size < SB_SIZE where SB_SIZE = 16, but growMemory does?

function searchBlock(root: Root, size: usize): Block | null {
// size was already asserted by caller
// mapping_search
let fl: usize, sl: u32;
if (size < SB_SIZE) {
fl = 0;
sl = <u32>(size >> AL_BITS);
} else {
const halfMaxSize = BLOCK_MAXSIZE >> 1; // don't round last fl
const inv: usize = sizeof<usize>() * 8 - 1;
const invRound = inv - SL_BITS;
let requestSize = size < halfMaxSize
? size + (1 << (invRound - clz<usize>(size))) - 1
: size;
fl = inv - clz<usize>(requestSize);
sl = <u32>((requestSize >> (fl - SL_BITS)) ^ (1 << SL_BITS));
fl -= SB_BITS - 1;
}

If so, perhaps instead of reordering the logic, mirroring exactly what searchBlock does would be preferable, that is by not performing the rounding if size < 16?

@HerrCai0907
Copy link
Member Author

ping @dcodeIO @MaxGraey

@dcodeIO dcodeIO merged commit 382aabe into AssemblyScript:main Jul 18, 2023
@HerrCai0907 HerrCai0907 deleted the fix/rt-grow-memory branch July 19, 2023 00:53
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