Freespace and finalization support in extent, using new metadata scheme #311
Conversation
Note: the extent side support split into #311
sdlib/d/gc/extent.d
Outdated
| auto base = nativeToBigEndian(native); | ||
| auto mask = -isLarge | nativeToBigEndian!ushort(ushort(0xff)); | ||
|
|
||
| auto current = *ptr; | ||
| auto delta = (current ^ base) & mask; | ||
| auto value = current ^ delta; | ||
| auto value = (current ^ delta) | (current & finalizerBit); |
There was a problem hiding this comment.
Change the mask instead of fetching the finalizer bit and you'll do this for free.
There was a problem hiding this comment.
good point, thank you.
|
Let's split the packign/unpackig of the metadata in its own MR. We can use the new format even if we never using finalization. |
sdlib/d/gc/extent.d
Outdated
| @@ -322,6 +322,13 @@ public: | |||
| return _metadata.slabData.freeSpaceData.freeSpaceFlags; | |||
| } | |||
|
|
|||
| bool hasFreeSpaceField(uint index) { | |||
There was a problem hiding this comment.
The whole point of this diff is that this isn't a free space field anymore.
sdlib/d/gc/extent.d
Outdated
| return 0; | ||
| } | ||
|
|
||
| // Decode freespace, found in the final byte (or two bytes) of the alloc: | ||
| return readPackedFreeSpace(freeSpacePtr(index)); | ||
| } | ||
|
|
||
| size_t getTotalSpace(uint index) { |
sdlib/d/gc/extent.d
Outdated
| "freeSpace must be set before finalizer!"); | ||
|
|
||
| if (finalizer is null) { | ||
| *(finalizerPtr(index)) &= ~FinalizerBit; |
There was a problem hiding this comment.
you are callign that functon in all branches and sometime several times.
sdlib/d/gc/extent.d
Outdated
| */ | ||
|
|
||
| @property | ||
| bool supportsFreeSpace() const { | ||
| bool supportsSlabMetaData() const { |
There was a problem hiding this comment.
Is this API used outside of contracts here? I have a hard time imagining it would in a way that's not a bug as the pd also has the size class.
If that's the case, it's probably preferable to inline isAppendableSizeClass in the contract themselves.
It would also be nice to rename this, because it's not just appenable anymore.
sdlib/d/gc/extent.d
Outdated
| auto finalizerFieldPtr = finalizerPtr(index); | ||
| if (!(*finalizerFieldPtr & FinalizerBit)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
You may want to check that first, so you avoid the atomic check.
sdlib/d/gc/extent.d
Outdated
| } | ||
|
|
||
| auto finalizerFieldPtr = finalizerPtr(index); | ||
| if (!(*finalizerFieldPtr & FinalizerBit)) { |
There was a problem hiding this comment.
Well int hat case you'd be better off with a == 0 instead of a bang.
sdlib/d/gc/extent.d
Outdated
| assert(isSlab(), "setFinalizer accessed on non slab!"); | ||
| assert(index < slotCount, "index is out of range!"); | ||
| assert(hasSlabMetaData(index), | ||
| "No metadata! (must set freespace before finalizer)"); |
There was a problem hiding this comment.
You want to check that finalizer & Addressmask == finalizer.
sdlib/d/gc/extent.d
Outdated
| } | ||
|
|
||
| auto newMetaData = (*finalizerFieldPtr & ~AddressMask) | FinalizerBit; | ||
| *finalizerFieldPtr = newMetaData | cast(ulong) cast(void*) finalizer; |
There was a problem hiding this comment.
All the various longs all over the place should be size_t, the size of a pointer.
sdlib/d/gc/extent.d
Outdated
| return 0; | ||
| } | ||
|
|
||
| // Decode freespace, found in the final byte (or two bytes) of the alloc: | ||
| return readPackedFreeSpace(freeSpacePtr(index)); | ||
| } | ||
|
|
||
| size_t getTotalCapacity(uint index) { |
There was a problem hiding this comment.
I'm not sure why the capacity has to be total. What else could it be? If you insist on qualifying, then maybe getSlotCapacity, but getCapacity is probably fine, the index kinda gives it away that it's about a specific slot.
sdlib/d/gc/extent.d
Outdated
| assert(index < slotCount, "index is out of range!"); | ||
|
|
||
| return hasSlabMetaData(index) | ||
| && (*(finalizerPtr(index)) & FinalizerBit); |
There was a problem hiding this comment.
I'd say you want to do the checks int he opposite order, this way you can avoid snooping in the metadata when you don't have to.
There was a problem hiding this comment.
In fact, this is likely to be false, consider that:
- If the slot is not full, then this is probably going to be zero.
- If it has metadata, in most cases, it won't have a finalizer: 0.
- If there is a pointer in there, it'll the MSB, which are going to be zero.
- If there is a number there, once again, MSB, and most numbers are small, so it's likely to be zero.
- If there is something else in there that don't heavily bias toward zero (float, random/compressed data) we are still at 50/50.
We should expect to find a zero in there the VAST majority of the time.
There was a problem hiding this comment.
BTW, this probably warrant a comment :P
sdlib/d/gc/extent.d
Outdated
| return PointerSize; | ||
| } | ||
|
|
||
| enum FinalizerBit = nativeToBigEndian!size_t(size_t(0x2)); |
There was a problem hiding this comment.
You always use a constructor here because somehow VRP doesn't kick in when calling the function, right?
There was a problem hiding this comment.
Correct for the one in the test, but it seems to default to size_t in fact. This one can be cut
Supercedes #307 .
Resolves #268 .