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

Improve stub RT reallocation #790

Merged
merged 2 commits into from
Aug 28, 2019
Merged

Improve stub RT reallocation #790

merged 2 commits into from
Aug 28, 2019

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Aug 23, 2019

potentially fixes #788
potentially fixes #789

Previously, if a memory block didn't fit anymore upon __realloc, the stub RT would reallocate to exactly the requested size, which is super inefficient since stub can't free and would waste lots of memory in any scenario reallocating multiple times (e.g. when filling a buffer). Instead, it now tries to at least double the capacity when wasting memory, to subsequently waste less.

@MaxGraey
Copy link
Member

MaxGraey commented Aug 24, 2019

btw. Analysing static-array.optimized.wat I see all memory runtime methods but this test use --runtime none and static arrays so ideally it should use only start, abort and Array<i32>#__get

@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 24, 2019

Seems the chain there is: __alloc called by __realloc called by ensureSize called by Array<T>#__set called by i[0] = 2 in the test, since indexed set can grow and as such is somewhat similar to push.

@MaxGraey
Copy link
Member

Yeah. Forgot about Array<T>#__set=)

@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 24, 2019

Now also deals with last blocks, either

  • growing the segment incl. memory/offset to avoid scenarios where memory is wasted by doubling and copying even if there's actually no next block that would make copying necessary - or -

  • shrinking the offset again so subsequent reallocs can still recognize the last block

@dcodeIO
Copy link
Member Author

dcodeIO commented Aug 28, 2019

Going to merge if no objections. @MaxGraey would you mind taking a look before I do?

@dcodeIO dcodeIO requested a review from MaxGraey August 28, 2019 17:57
Copy link
Member

@MaxGraey MaxGraey left a comment

Choose a reason for hiding this comment

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

LGTM

@dcodeIO dcodeIO merged commit 9ba8162 into master Aug 28, 2019
if (size > BLOCK_MAXSIZE) unreachable();
maybeGrowMemory(ptr + size);
} else { // copy to new block at least double the size
let newPtr = __alloc(max<usize>(size, actualSize << 1), block.rtId);

Choose a reason for hiding this comment

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

How does this work?
We allocate max<usize>(size, actualSize << 1), but in line 56 we set block.rtSize = size, so the next call to realloc will look at previous size and not the allocated capacity, and allocate again.
What am I missing?

@dcodeIO dcodeIO deleted the issue-788 branch November 8, 2019 02:00
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.

Array#push causes memory problems
3 participants