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

Resize-in-place for large allocs. (Shrink-only) #289

Merged
merged 54 commits into from
Sep 8, 2023

Conversation

dsm9000
Copy link
Contributor

@dsm9000 dsm9000 commented Sep 4, 2023

In re: #265 .
realloc() now resizes in place when possible, using appropriate size class.
Currently only supports realloc() downwards.

sdlib/d/gc/tcache.d Outdated Show resolved Hide resolved
sdlib/d/gc/tcache.d Outdated Show resolved Hide resolved
sdlib/d/gc/arena.d Outdated Show resolved Hide resolved
sdlib/d/gc/emap.d Outdated Show resolved Hide resolved
sdlib/d/gc/emap.d Outdated Show resolved Hide resolved
sdlib/d/gc/hpd.d Outdated Show resolved Hide resolved
sdlib/d/gc/hpd.d Outdated Show resolved Hide resolved
sdlib/d/gc/arena.d Outdated Show resolved Hide resolved
sdlib/d/gc/arena.d Outdated Show resolved Hide resolved
sdlib/d/gc/arena.d Outdated Show resolved Hide resolved
sdlib/d/gc/arena.d Outdated Show resolved Hide resolved
sdlib/d/gc/arena.d Outdated Show resolved Hide resolved
sdlib/d/gc/arena.d Outdated Show resolved Hide resolved
@dsm9000 dsm9000 marked this pull request as ready for review September 4, 2023 19:11
Comment on lines 167 to 168
uint oldSize = cast(uint) e.size;
uint oldPages = oldSize / PageSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

The cast should be on oldPages. Also, it's the currentSize.


auto computedPageCount = alignUp(size, PageSize) / PageSize;
uint newPages = computedPageCount & uint.max;

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove that empty line. the assert being on its own is quite confusing as it's not obvious that it is just a sanity check on the previous computation (check that there is no truncation going on, which should be impossible considering the allocatable size we have).

sdlib/d/gc/arena.d Outdated Show resolved Hide resolved
Comment on lines 402 to 403
e.at(e.address, newPages * PageSize, hpd);
hpd.clear(n + newPages, oldPages - newPages);
Copy link
Contributor

Choose a reason for hiding this comment

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

This tells me what you actually need as input is e, n + newPages, newPages, delta. It's 4 just as before, but it maps to what you actually need in there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably named e, index, pages, delta.

@deadalnix
Copy link
Contributor

This is looking good, but there is probably some simplification in argument passing in the various functions in the arena.

assert(isLargeSize(size));

// Huge is not supported:
if (e.isHuge()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this happen if the isLarge assert pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, please see definition, isLarge includes any extent that isn't a slab.

@@ -338,6 +392,22 @@ private:
unusedExtents.insert(e);
}

void shrinkAllocImpl(Extent* e, uint index, uint pages, uint delta) {
assert(mutex.isHeld(), "Mutex not held!");
assert(index <= PageCount - pages, "Invalid index!");
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but index > 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, fixed.

Comment on lines 166 to 167
auto currentSize = e.size;
uint oldPages = cast(uint) (currentSize / PageSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

e.pageCount

Comment on lines 283 to 284
uint currentPages = cast(uint) (e.size / PageSize);
assert(currentPages > pages, "Invalid shrink pages!");
Copy link
Contributor

Choose a reason for hiding this comment

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

e.pageCount

Comment on lines +401 to +408
auto hpd = e.hpd;
unregisterHPD(hpd);

e.at(e.address, pages * PageSize, hpd);
hpd.clear(index, delta);

assert(!hpd.empty);
registerHPD(hpd);
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 great how clean this became.


uint delta = currentPages - pages;

uint n = ((cast(size_t) e.address) / PageSize) % PageCount;
Copy link
Contributor

Choose a reason for hiding this comment

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

e.hpdIndex

Comment on lines 161 to 163
auto computedPageCount = alignUp(size, PageSize) / PageSize;
uint newPages = computedPageCount & uint.max;
assert(newPages == computedPageCount, "Unexpected page count!");
Copy link
Contributor

Choose a reason for hiding this comment

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

getPageCount

auto p4 = threadCache.realloc(p3, 16000, false);
assert(p4 !is p3);
assert(p4 is p3);
assert(threadCache.getCapacity(p4[0 .. 16000]) == 16384);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test shrink from large to small?

@deadalnix deadalnix merged commit 1c07be3 into snazzy-d:master Sep 8, 2023
3 checks passed
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