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

allocAppendable() and getCapacity(). #284

Merged
merged 35 commits into from
Aug 24, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
3a28488
bare-bones appendables (not tested yet)
dsm9000 Aug 15, 2023
bb26af1
tests; recommended corrections; afaik everything but adjustment on re…
dsm9000 Aug 15, 2023
6cdf9b7
realloc handling for appendables; tests for same
dsm9000 Aug 15, 2023
2afbad9
realloc handling for appendables; tests for same
dsm9000 Aug 15, 2023
781dd37
realloc handling for appendables; tests for same
dsm9000 Aug 15, 2023
af06b86
realloc handling for appendables; tests for same
dsm9000 Aug 15, 2023
fb86c4f
realloc handling for appendables; tests for same
dsm9000 Aug 15, 2023
20850f6
realloc handling for appendables; tests for same (simplified)
dsm9000 Aug 15, 2023
6bcd254
realloc handling for appendables; tests for same (simplified)
dsm9000 Aug 15, 2023
0de7276
realloc handling for appendables; tests for same (simplified)
dsm9000 Aug 15, 2023
2e782e6
realloc handling for appendables (with tests for unknown-to-gc ptrs)
dsm9000 Aug 16, 2023
43b61bd
realloc handling for appendables (with tests for unknown-to-gc ptrs)
dsm9000 Aug 16, 2023
852d98f
corrections
dsm9000 Aug 16, 2023
960ccf6
Merge branch 'master' of github.com:dsm9000/sdc into basic_appendables
dsm9000 Aug 17, 2023
e5c753e
in-place appending api.
dsm9000 Aug 21, 2023
5c276ff
in-place appending api.
dsm9000 Aug 21, 2023
23abbcd
in-place appending api.
dsm9000 Aug 21, 2023
e697fba
in-place appending api. (corrections)
dsm9000 Aug 21, 2023
d4d9899
in-place appending api. (corrections, tests)
dsm9000 Aug 21, 2023
3b439dd
in-place appending api. (corrections, tests)
dsm9000 Aug 21, 2023
f8d87ba
getCapacity(), realloc(), and tests for same; support for initially-e…
dsm9000 Aug 22, 2023
2037ca8
getCapacity(), realloc(), and tests for same; support for initially-e…
dsm9000 Aug 22, 2023
103e7fe
getCapacity(), realloc(), and tests for same; support for initially-e…
dsm9000 Aug 22, 2023
978238c
getCapacity(), realloc(), and tests for same; support for initially-e…
dsm9000 Aug 22, 2023
54dcc43
getCapacity(), realloc(), and tests for same; support for initially-e…
dsm9000 Aug 22, 2023
ef01fee
getCapacity(), realloc(), and tests for same; support for initially-e…
dsm9000 Aug 22, 2023
9066a34
getCapacity() docs
dsm9000 Aug 22, 2023
4e7db41
getCapacity() docs
dsm9000 Aug 22, 2023
f15d0ac
getCapacity() docs
dsm9000 Aug 22, 2023
5bce071
getCapacity() docs
dsm9000 Aug 22, 2023
eac9095
getCapacity() docs
dsm9000 Aug 22, 2023
2cd70e3
getCapacity() corrections
dsm9000 Aug 23, 2023
0c73f63
various corrections
dsm9000 Aug 23, 2023
514481e
various corrections
dsm9000 Aug 23, 2023
733feca
corrections and simplifications suggested by deadalnix.
dsm9000 Aug 24, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions sdlib/d/gc/extent.d
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,15 @@ private:
Links _links;

import d.gc.bitmap;
Bitmap!512 _slabData = void;
union Meta {
Copy link
Contributor

Choose a reason for hiding this comment

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

Metadata

// Slab occupancy (constrained by freeSlots, so usable for all classes)
Bitmap!512 _slabData;
Copy link
Contributor

Choose a reason for hiding this comment

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

The underscore isn't necessary here anymore.


// Metadata for non-slab (large) size classes
size_t allocSize; // Actual alloc size, 0 if not appendable
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not put comment on the same line as declarations.

}

Meta _meta;

this(uint arenaIndex, void* ptr, size_t size, ubyte generation,
HugePageDescriptor* hpd, ExtentClass ec) {
Expand All @@ -120,10 +128,27 @@ private:
bits |= ulong(binInfos[ec.sizeClass].slots) << 48;

slabData.clear();
} else {
_meta.allocSize = 0;
}
}

public:
@property
bool isAppendable() {
return allocSize != 0;
}

@property
ulong allocSize() {
return isLarge() ? _meta.allocSize : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

It clearly a bug to call this on slabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well no, because e.g. is_appendable and the related API functions can very well end up being called on a slab alloc, and must return the correct answer (i.e. that the alloc is not appendable, and appendability operations are prohibited.)

}

void setAllocSize(size_t size) {
assert(isLarge(), "Cannot set alloc size on a slab alloc!");
_meta.allocSize = size;
}

Extent* at(void* ptr, size_t size, HugePageDescriptor* hpd,
ExtentClass ec) {
this = Extent(arenaIndex, ptr, size, generation, hpd, ec);
Expand Down Expand Up @@ -202,6 +227,10 @@ public:
return ec.isSlab();
}

bool isLarge() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that in the middle of slab features? Make a large features section and put the large feature related stuff in it.

return !isSlab();
}

@property
ubyte sizeClass() const {
auto ec = extentClass;
Expand Down Expand Up @@ -240,7 +269,7 @@ public:
// FIXME: in contract.
assert(isSlab(), "slabData accessed on non slab!");

return _slabData;
return _meta._slabData;
}
}

Expand Down
155 changes: 146 additions & 9 deletions sdlib/d/gc/tcache.d
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module d.gc.tcache;

import d.gc.extent;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

import d.gc.sizeclass;
import d.gc.spec;
import d.gc.util;
Expand All @@ -15,17 +16,26 @@ private:
const(void*)[][] roots;

public:
void* alloc(size_t size, bool containsPointers) {
void* alloc(size_t size, bool containsPointers, bool isAppendable = false) {
if (!isAllocatableSize(size)) {
return null;
}

initializeExtentMap();

auto arena = chooseArena(containsPointers);
return size <= SizeClass.Small
? arena.allocSmall(emap, size)
: arena.allocLarge(emap, size, false);

if (isAppendable) {
// Currently, large (extent) allocs must be used for appendables:
auto ptr = arena.allocLarge(emap, upsizeToLarge(size), false);
auto pd = getPageDescriptor(ptr);
pd.extent.setAllocSize(size);
return ptr;
} else {
return size <= SizeClass.Small
? arena.allocSmall(emap, size)
: arena.allocLarge(emap, size, false);
}
}

void* calloc(size_t size, bool containsPointers) {
Expand Down Expand Up @@ -54,6 +64,39 @@ public:
pd.arena.free(emap, pd, ptr);
}

// Determine whether given alloc is appendable
bool is_appendable(void* ptr) {
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 used anywhere, and is just a helper around another function, that doesn't seems worth including.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intended to be part of the exported API for appendables, rather than a helper.
What precisely do we intend to do with appendables anyway? This was not discussed yet.

return maybeGetAppendableExtent(ptr) !is null;
}

// Get the current fill of an appendable alloc.
// If the alloc is not appendable, returns 0.
size_t get_appendable_fill(void* ptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work with appendable pointer. This API is still a more or less random set of features that do not seems to be connected with the actual use case.

The actual use case is that D has slices, and slices can be appended to, and reallocations needs to be avoided when unnecessary.

I don't know how to do any of this with the current API, considering it doesn't work with interior pointers.

So, what happens when I allocate a slice? What happens when I want to append to a slice? What happens when the slice is not on the GC heap? This is what matters here. I have no API do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I interpreted #266 as concerning the GC strictly, rather than connecting the mechanism with the rest of SDC. This may be the root of our misunderstanding and the reason you've replied with "random set of features that do not seems to be connected" to all of my PR. Could I persuade you to fully describe the requirements for satisfying #266 ? Slices, for instance, were mentioned nowhere in #266.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"The API of the GC needs to be extended with a method that allows us to query the size stored via the appendable mechanism."

This is all that was clear from #266 as originally given.

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 slices: as I understand, if we're doing an append operation on a slice, we'll get the pointer to the parent array from the GC heap (or from the calling code, which already has it, when we're not doing so from the GC.)

auto e = maybeGetAppendableExtent(ptr);
return (e !is null) ? e.allocSize : 0;
}

// Get the current free space of an appendable alloc.
// If the alloc is not appendable, returns 0.
size_t get_appendable_free_space(void* ptr) {
auto e = maybeGetAppendableExtent(ptr);
return (e !is null) ? e.size - e.allocSize : 0;
}

// Change the current fill of an appendable alloc.
// If the alloc is not appendable, or the requested
// fill exceeds the available space, returns false.
bool set_appendable_fill(void* ptr, size_t fill) {
auto e = maybeGetAppendableExtent(ptr);

if ((e !is null) && (fill <= e.size)) {
e.setAllocSize(fill);
return true;
}

return false;
}

void* realloc(void* ptr, size_t size, bool containsPointers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the C API together.

if (!isAllocatableSize(size)) {
free(ptr);
Expand All @@ -64,9 +107,12 @@ public:
return alloc(size, containsPointers);
}

size_t oldFill = 0;
auto copySize = size;
auto pd = getPageDescriptor(ptr);

// TODO: should realloc work with pointers outside of GC?
Copy link
Contributor

Choose a reason for hiding this comment

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

How would it know how big the previous allocation is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It couldn't. That comment should go.


if (pd.isSlab()) {
auto newSizeClass = getSizeClass(size);
auto oldSizeClass = pd.sizeClass;
Expand All @@ -78,23 +124,37 @@ public:
copySize = getSizeFromClass(oldSizeClass);
}
} else {
auto esize = pd.extent.size;
if (alignUp(size, PageSize) == esize) {
oldFill = pd.extent.allocSize;

// Prohibit resize below appendable fill:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a sensible design decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why isn't it? If we don't have a stored fill, we have to copy the entire alloc space when reallocing.

if (size < oldFill)
return ptr;

auto oldSize = oldFill ? oldFill : pd.extent.size;

if (alignUp(size, PageSize) == alignUp(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.

You are short circuiting the whole thing when the size you'd get is the same. Yet, later on, you recompute the size.

return ptr;
}

// TODO: Try to extend/shrink in place.
import d.gc.util;
copySize = min(size, esize);
copySize = min(size, oldSize);
}

containsPointers = (containsPointers | pd.containsPointers) != 0;
auto newPtr = alloc(size, containsPointers);
auto useSize = oldFill ? upsizeToLarge(size) : size;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you recompute the size, so the previous short circuit is incorrect. This will also lead to quadratic behavior as the size will only be bumped up one page at the time instead of exponentially, which is linear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What kind of bump increment do we want for realloc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incidentally upsizeToLarge does not recompute the size unless the offered size is below the minimum for a large alloc. The size passed to alloc still needs to be aligned.

auto newPtr = alloc(useSize, containsPointers);
if (newPtr is null) {
return null;
}

memcpy(newPtr, ptr, copySize);

if (oldFill) {
auto pdNew = getPageDescriptor(newPtr);
assert(pdNew.extent.isLarge());
pdNew.extent.setAllocSize(oldFill);
}

pd.arena.free(emap, pd, ptr);

return newPtr;
Expand Down Expand Up @@ -171,6 +231,29 @@ public:
}

private:
// Force large allocation rather than slab
size_t upsizeToLarge(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.

Why is this a member method? This doesn't use any of the data that's in this struct, nor does it call any of its methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to be in sizeclass.d.

auto aSize = size;
while (getAllocSize(aSize) <= SizeClass.Small) {
Copy link
Contributor

Choose a reason for hiding this comment

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

none of the functions require looping in any way. This code is going to be called on every allocations, why is this looping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This routine will be unnecessary once we support appendability for slab allocs.
It is possible to do this more efficiently, but can we first settle the full requirements for satisfying #266 please?

aSize = getSizeFromClass(getSizeClass(aSize) + 1);
}

return aSize;
}

// Get appendable extent of ptr, or null if this is impossible
Extent* maybeGetAppendableExtent(void* ptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this method necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is needed because any invocation of appendable API could have been given a non-appendable (slab alloc, or pointer unknown to GC.)

if (ptr is null)
return null;

auto pd = maybeGetPageDescriptor(ptr);
if ((pd.extent is null) || (ptr !is pd.extent.address)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we check for the address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the API functions may be called on a slab alloc or a pointer unknown to the GC. Please see last section of the unit test.

|| (!pd.extent.isAppendable))
return null;

return pd.extent;
}

auto getPageDescriptor(void* ptr) {
auto pd = maybeGetPageDescriptor(ptr);
assert(pd.extent !is null);
Expand Down Expand Up @@ -283,3 +366,57 @@ unittest makeRange {
checkRange(ptr[1 .. 7], 0, 0);
checkRange(ptr[1 .. 8], 8, 8);
}

unittest appendableAlloc {
dsm9000 marked this conversation as resolved.
Show resolved Hide resolved
// Basics:
auto p0 = threadCache.alloc(100, false, true);
auto p1 = threadCache.alloc(100, false, false);
assert(threadCache.is_appendable(p0));
assert(!threadCache.is_appendable(p1));
assert(threadCache.get_appendable_fill(p1) == 0);
assert(threadCache.get_appendable_free_space(p1) == 0);
assert(threadCache.get_appendable_fill(p0) == 100);
assert(threadCache.get_appendable_free_space(p0) == 16284);
assert(threadCache.set_appendable_fill(p0, 50000) == false);
assert(threadCache.set_appendable_fill(p0, 200) == true);
assert(threadCache.get_appendable_fill(p0) == 200);
assert(threadCache.get_appendable_free_space(p0) == 16184);

// Realloc non-appendable alloc, should stay non-appendable :
p1 = threadCache.realloc(p1, 1000, false);
assert(p1 !is null);
assert(!threadCache.is_appendable(p1));
assert(threadCache.get_appendable_fill(p1) == 0);
assert(threadCache.get_appendable_free_space(p1) == 0);
threadCache.free(p1);

// Realloc an appendable alloc up :
p0 = threadCache.realloc(p0, 100000, false);
assert(p0 !is null);
assert(threadCache.get_appendable_fill(p0) == 200);
assert(threadCache.set_appendable_fill(p0, 50000) == true);
assert(threadCache.get_appendable_fill(p0) == 50000);
assert(threadCache.get_appendable_free_space(p0) == 52400);

// Realloc an appendable alloc down :
p0 = threadCache.realloc(p0, 60000, false);
assert(p0 !is null);
assert(threadCache.get_appendable_free_space(p0) == 11440);

// Realloc prohibited by appendable fill, so nothing should change
auto p3 = threadCache.realloc(p0, 40000, false);
assert(p3 == p0);

// Pointers the GC does not know about:
void* interior = p0 + 2000;
assert(!threadCache.is_appendable(interior));
assert(threadCache.set_appendable_fill(interior, 222) == false);
assert(threadCache.get_appendable_fill(interior) == 0);
assert(threadCache.get_appendable_free_space(interior) == 0);
threadCache.free(p0);
auto martian = cast(void*) 0x56789abcd000;
assert(!threadCache.is_appendable(martian));
assert(threadCache.set_appendable_fill(martian, 333) == false);
assert(threadCache.get_appendable_fill(martian) == 0);
assert(threadCache.get_appendable_free_space(martian) == 0);
}