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

fxGrowChunks making very small allocations #636

Closed
warner opened this issue May 3, 2021 · 2 comments
Closed

fxGrowChunks making very small allocations #636

warner opened this issue May 3, 2021 · 2 comments
Labels
confirmed issue reported has been reproduced

Comments

@warner
Copy link

warner commented May 3, 2021

I've been investigating performance problems in our xsnap usage of XS, where we're seeing GC happen more frequently than expected, and the allocations being made to fxAllocateChunks are surprisingly small (32-4000 bytes at a time), even though our xsCreation settings do incrementalHeapCount = 256 * 1024.

I'm suspicious of the following code in fxGrowChunks:

void fxGrowChunks(txMachine* the, txSize theSize)
{
txByte* aData;
txBlock* aBlock;
if (!(the->collectFlag & XS_SKIPPED_COLLECT_FLAG)) {
txSize modulo = theSize % the->minimumChunksSize;
if (modulo)
fxAddChunkSizes(the, theSize, the->minimumChunksSize - modulo);
}
theSize = fxAddChunkSizes(the, theSize, sizeof(txBlock));
aData = fxAllocateChunks(the, theSize);

It looks like the return value from fxAddChunkSizes() is being ignored, leaving theSize at its original value (e.g. 28 when allocating a new Function). On 17-Mar-2021, in 3070f24, this was changed:

3070f24#diff-5458ac58321f0e9dcd9ea60d1b7e2b87ea74e00cb70c5da666d1eed076c14258R380

I think the original intention was to increase theSize to the->minimumChunksSize, give or take some rounding and headers. In the new version, I think that increase got lost, resulting in small allocations, and therefore more frequent GC calls.

The fix is probably just:

	theSize = fxAddChunkSizes(the, theSize, the->minimumChunksSize - modulo);

We're building against a derivative of commit 0b451c9 (aka tag OS210406), on Linux.

thanks!
-Brian

@phoddie
Copy link
Collaborator

phoddie commented May 3, 2021

Good find. That's our bug. We'll apply that to our next SDK update. Thank you for the help. Apologies for the trouble.

@phoddie phoddie added the confirmed issue reported has been reproduced label May 3, 2021
dckc pushed a commit to agoric-labs/moddable that referenced this issue May 4, 2021
mkellner pushed a commit that referenced this issue May 4, 2021
@phoddie
Copy link
Collaborator

phoddie commented Jul 28, 2021

Closing this issue, as I believe it has been resolved.

@phoddie phoddie closed this as completed Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed issue reported has been reproduced
Projects
None yet
Development

No branches or pull requests

2 participants