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

Batch alloc from bin with behaviour patterned after jemalloc's #334

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsm9000
Copy link
Contributor

@dsm9000 dsm9000 commented Oct 24, 2023

No description provided.

@dsm9000 dsm9000 force-pushed the batch_alloc_from_bin branch 5 times, most recently from 9b27c78 to 6bd2c62 Compare October 24, 2023 04:15
@dsm9000 dsm9000 marked this pull request as draft October 24, 2023 14:16
@dsm9000 dsm9000 marked this pull request as ready for review October 24, 2023 14:55
@dsm9000 dsm9000 force-pushed the batch_alloc_from_bin branch 2 times, most recently from 88c246a to f4e8974 Compare October 24, 2023 18:09
@dsm9000 dsm9000 marked this pull request as draft October 24, 2023 18:32
@dsm9000 dsm9000 marked this pull request as ready for review October 24, 2023 18:56
return buffer[0];
}

uint allocSmallBatch(shared(ExtentMap)* emap, size_t size,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the batch small? The good news is that it doesn't matter much: this is not claled, you can just tweak allocSmall and be done with it.

sdlib/d/gc/bin.d Outdated
Comment on lines 45 to 48
auto nFree = slab.freeSlots;
if (nFree == 0) {
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would that happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it, AFAIK it can't, even on OOM. That part ought to go

sdlib/d/gc/bin.d Outdated
Comment on lines 50 to 51
auto toFill = needFill - filled;
auto count = toFill < nFree ? toFill : nFree;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary.

sdlib/d/gc/bin.d Outdated
auto count = toFill < nFree ? toFill : nFree;
auto gotSlots =
slab.batchAllocate(buffer[filled .. filled + count], size);
filled += gotSlots;
Copy link
Contributor

Choose a reason for hiding this comment

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

filled += slab.batchAllocate(buffer[filled .. buffer.length], size);

sdlib/d/gc/bin.d Outdated
@@ -30,20 +30,31 @@ struct Bin {
import d.gc.slab;
auto size = binInfos[sizeClass].itemSize;

auto needFill = buffer.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you plan on filling the buffer, then the best is likely to slice the remaining part of the buffer at each iterration and check that the size of the buffer is greater than 0 int he loop.

But I'm wondering if we are not better off having a minimal number of allocation requested as a parameter, and possibly a bigger buffer, so that we can fill in on a slab granularity.

Copy link
Contributor

Choose a reason for hiding this comment

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

The latter strategy is done by other allocators, but not jemalloc, I'm not sure why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't filling in on a slab granularity require a rather large minimal cache bin size? At least for the smaller size classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

512 items for small size classes do not seem absurd.

Copy link
Contributor Author

@dsm9000 dsm9000 Oct 25, 2023

Choose a reason for hiding this comment

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

So: cache 1 slab's worth per class ? sounds good imho (or maybe 1 page's worth?)

@deadalnix
Copy link
Contributor

It be more useful for anyone going over this code that the title describes what this does rather than the fact it does something similar to jemalloc, because then, one has not been enlightened much.

@@ -153,3 +164,64 @@ private:
return current;
}
}

unittest batchAllocation {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not testing interesting behavior, such as what happens when there are several partially filled slab in the bin.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the problem that is being dealt with here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

All the test are filling up available extents. We should see extents left behind and ensure that they are in the correct state.

sdlib/d/gc/bin.d Outdated

auto size = getSizeFromClass(sc);
auto gotSlots =
arena.allocSmallBatch(&emap, size, buffer[0 .. wantSlots]);
Copy link
Contributor

Choose a reason for hiding this comment

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

The entry point should be thing bin's batchAllocate .

sdlib/d/gc/bin.d Outdated
auto expectedSlabs = 4;

import d.gc.sizeclass;
foreach (sc; 0 .. ClassCount.Small) {
Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, checking all size class isn't really necessary. Checking a couple, with a couple of slot count could be useful.

@deadalnix
Copy link
Contributor

As mentioned in #334 (comment) , the title/description is not useful in its current shape.

if (slab is null) {
return null;
}
auto remainBuffer = buffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you need to create a local variable to track thing, it'd be simple for it to be the number of items allocated.

Copy link
Contributor

Choose a reason for hiding this comment

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

You'd avoid a cast and various computations.

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.

2 participants