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

Finalizer support in extent. #307

Closed
wants to merge 20 commits into from

Conversation

dsm9000
Copy link
Contributor

@dsm9000 dsm9000 commented Oct 6, 2023

No description provided.

sdlib/d/gc/extent.d Outdated Show resolved Hide resolved
assert(index < slotCount, "index is out of range!");
assert(hasFinalizer(index), "No finalizer is set!");

return cast(void*) (*last64Bits >> 16 & AddressMask);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, it is in the free space because you shift, but it's not if you don't shift. (and, incidentally, if you don't shift, it where it always was).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the idea here is that now the encoded freespace is to be found below the finalizer (if one exists), and so can occupy 1 byte if fits in 1 byte, or 0 bytes if there is no freespace (as it did before introducing finalizers.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deadalnix AFAIK this piece is complete; can merge? Or do you see any further issues? (e.g. would you prefer that the 2 final bytes get always reserved for the freespace when finalizer is enabled, to trade the space saving for a small speedup?)

cast(ushort*) (address + getUsableSpace(index) - 2));
}

size_t getUsableSpace(uint 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 only that shouldn't really be necessary, but this is wrong. This is the second time incorrect code is in there and passes tests. So I'll let add tests for boundary condition and break this down until it is obvious none of these remain. Relying on me to find all the problem is not a solution.

@@ -100,6 +100,22 @@ unittest isAppendableSizeClass {
}
}

// Determine whether given size class supports finalization.
bool isFinalizableSizeClass(uint sizeClass) {
return (getSizeFromClass(sizeClass) & 0x1f) == 0;
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 probably hopelessly inefficient.

assert(index < slotCount, "index is out of range!");
assert(hasFinalizer(index), "No finalizer is set!");

return cast(void*) (*last64Bits >> 16);
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -806,3 +806,38 @@ unittest arraySpill {
testSpill(16384, [0, 1, 2, 500, 16000, 16383, 16384]);
testSpill(20480, [0, 1, 2, 500, 20000, 20479, 20480]);
}

unittest finalizers {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no change in tcache, why is there a test in tcache?

@dsm9000
Copy link
Contributor Author

dsm9000 commented Oct 12, 2023

Superceded by #311

@dsm9000 dsm9000 closed this Oct 12, 2023
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