-
-
Notifications
You must be signed in to change notification settings - Fork 655
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
~lib/array/Array#push
is unnecessarily garbage-y
#1798
Comments
That's well known problem with incremental runtime and constantly grown array (via serial of push for example). |
That's indeed odd, yeah. Looking at the code again, |
I don’t know what FL or SL are 🙈 What I don’t think either of you have answered: Are you opposed to adjusting the |
It's a regression that it currently doesn't do this (anymore, not sure when exactly I messed this up), so absolutely, go for it. If we can tweak the factor a bit, the better. The only place where we may not want arrays to grow like this is when setting One nice-to-have aspect there would be to also take block size of TLSF into account, so instead of growing unconditionally when length is exceeded, arrays would only need to grow when the TLSF block cannot be extended anymore. For instance, when a block's payload is just one byte large as would be the case for an array of just one FL and SL are the first and second level lists of TLSF, indicating block size. The payload size of a block is somewhere in between where the previous list would end and the next list would start, so that's where we can just modify the block to consider a larger payload while not needing to alloc a new one. Current size of the block is in |
Could be just easily fix from: var newLength = length + 1; to var newLength = min(16, length << 1); here: |
Is it that simple? 😅 I think that would still cause a reallocation on every push, just with a bigger buffer size so that half of it is unused. |
Something like:
ensures (given an unsigned integer) a power of 2 bigger or equal of the starting value is returned. Note that calling only on entrance (=push codepath) solves the problem only partially, since also the [] path has to be checked, and seems more solid to do inside |
if you need Problem with growing to next pow of 2 is much more significant non-used memory footprint |
Thanks for pointing out the mistake.
|
I guess we could test / bench 3 solutions:
|
Note that the bit math above (relying on assemblyscript/std/assembly/rt/tlsf.ts Lines 5 to 9 in 7e20ad2
|
I figured out that function log2(n: u32): u32 {
return 31 - clz(n | 1);
}
function computeGrowFactor(len: u32): u32 {
if (len < (1 << 16)) {
return len * (1 + log2(len));
} else {
return min(len << 1, 1 << 30);
}
} In this case it takes only |
It seems I found the best empirical expansion (logarithmic): function computeGrowFactor(len: u32): u32 {
return len * (1 << (2 - ((log2(len - 8) - 8) >>> 4)));
} which produce this series - (length value, grow factor):
|
In a benchmark I discovered that AssemblyScript is significantly slower than JavaScript when using
--runtime incremental
when filling Arrays.A
d8
trace revealed the following profile:which led me to investigate the
Array#push()
implementation, which callsensureSize()
.ensureSize()
will create a new buffer with exactly one slot more if the previous buffer was full. This means that once you ran out of capacity once, every subsequent call topush()
will create a new buffer copy all the data to the new buffer and create work for the garbage collector as the old buffer is now collectible.Both Rust and Go amortize that work by doubling the capacity every time the buffer is exhausted. I’m sure C++ does the same but I couldn’t get through the stdlib code 😅
In an experiment, replacing the
~lib/array/Array
with aCustomArray<T>
that has exponential buffer growth, the performance problems go away:I would like to PR in a fix, but wanted to check first if there are any concerns about adopting a similar approach for ASC’s
Array<T>
?The text was updated successfully, but these errors were encountered: