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.

Sign up for GitHub

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

Mechanism to track currently-dirtiest region. #361

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

Mechanism to track currently-dirtiest region. #361

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
80 changes: 75 additions & 5 deletions sdlib/d/gc/region.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ import sdc.intrinsics;

alias ClassTree = RBTree!(Region, classAddrRegionCmp, "rbClass");
alias RangeTree = RBTree!(Region, addrRangeRegionCmp, "rbRange");
alias DirtTree = RBTree!(Region, dirtAddrRegionCmp, "rbDirt");

alias ClassNode = rbtree.Node!(Region, "rbClass");
alias RangeNode = rbtree.Node!(Region, "rbRange");
alias DirtNode = rbtree.Node!(Region, "rbDirt");
alias PHNode = heap.Node!Region;

// Reserve memory in blocks of 1GB.
Expand Down Expand Up @@ -59,6 +61,9 @@ private:
ClassTree regionsByClass;
RangeTree regionsByRange;

// Dirty regions only, by dirt (in descending order)
DirtTree regionsByDirt;

// Unused region objects.
Heap!(Region, unusedRegionCmp) unusedRegions;

Expand All @@ -77,6 +82,7 @@ public:
assert(blocks > 0, "Invalid number of blocks!");

// Eagerly clean the block we are returned.
// FIXME: remove when fine-grained purge works.
import d.gc.memmap;
pages_purge(ptr, blocks * BlockSize);

Expand Down Expand Up @@ -116,6 +122,7 @@ private:
}
} else {
regionsByRange.remove(r);
unregisterRegionDirt(r);
}

assert(r.address !is null && isAligned(r.address, BlockSize),
Expand Down Expand Up @@ -158,6 +165,22 @@ private:
registerRegion(r);
}

void registerRegionDirt(Region* r) {
assert(r !is null, "Region is null!");

if (r.dirtyBlockCount > 0) {
regionsByDirt.insert(r);
}
}

void unregisterRegionDirt(Region* r) {
assert(r !is null, "Region is null!");

if (r.dirtyBlockCount > 0) {
regionsByDirt.remove(r);
}
}

void registerRegion(Region* r) {
assert(r !is null, "Region is null!");

Expand All @@ -169,6 +192,7 @@ private:
}

regionsByClass.remove(adjacent);
unregisterRegionDirt(adjacent);

// Make sure we keep using the best region.
bool needSwap = unusedRegionCmp(r, adjacent) < 0;
Expand All @@ -181,6 +205,7 @@ private:

regionsByClass.insert(r);
regionsByRange.insert(r);
registerRegionDirt(r);
}

Region* refillAddressSpace(uint extraBlocks) {
Expand Down Expand Up @@ -369,6 +394,7 @@ struct Region {
struct UsedLinks {
ClassNode rbClass;
RangeNode rbRange;
DirtNode rbDirt;
}

union Links {
Expand Down Expand Up @@ -466,6 +492,11 @@ public:
return _links.usedLinks.rbRange;
}

@property
ref DirtNode rbDirt() {
return _links.usedLinks.rbDirt;
}

@property
size_t size() const {
return blockCount * BlockSize;
Expand Down Expand Up @@ -552,6 +583,23 @@ ptrdiff_t classAddrRegionCmp(Region* lhs, Region* rhs) {
return (l > r) - (l < r);
}

ptrdiff_t dirtAddrRegionCmp(Region* lhs, Region* rhs) {
// Descending order of dirt
auto rdirt = lhs.dirtyBlockCount;
auto ldirt = rhs.dirtyBlockCount;
auto dirtCmp = (ldirt > rdirt) - (ldirt < rdirt);

if (dirtCmp != 0) {
return dirtCmp;
}

// Descending order of address
auto r = cast(size_t) lhs.address;
auto l = cast(size_t) rhs.address;

return (l > r) - (l < r);
}

ptrdiff_t unusedRegionCmp(Region* lhs, Region* rhs) {
static assert(LgAddressSpace <= 56, "Address space too large!");

Expand Down Expand Up @@ -609,37 +657,59 @@ unittest trackDirtyBlocks {
assert(r.countDirtyBlocksInSubRegion(0, blocks) == dirtyBlocks);
}

// Verify that the currently-dirtiest region has the given attributes.
void verifyDirtiestRegion(void* address, uint blocks, uint dirtyBlocks) {
Region rr;
rr.dirtyBlockCount = uint.max;

auto r = ra.regionsByDirt.extractBestFit(&rr);
assert(r !is null);
assert(r.address is address);
assert(r.blockCount == blocks);
assert(r.dirtyBlockCount == dirtyBlocks);
assert(r.countDirtyBlocksInSubRegion(0, blocks) == dirtyBlocks);
ra.registerRegionDirt(r);
}

// Initially, there are no dirty blocks.
assert(regionAllocator.dirtyBlockCount == 0);

// Make some dirty regions.
freeRun(addresses[0 .. 2]);
assert(regionAllocator.dirtyBlockCount == 2);
verifyUniqueRegion(addresses[0], 2, 2, 2);
freeRun(addresses[4 .. 8]);
assert(regionAllocator.dirtyBlockCount == 6);
assert(regionAllocator.dirtyBlockCount == 4);
verifyUniqueRegion(addresses[4], 4, 4, 4);
verifyDirtiestRegion(addresses[4], 4, 4);
freeRun(addresses[10 .. 15]);
assert(regionAllocator.dirtyBlockCount == 11);
assert(regionAllocator.dirtyBlockCount == 9);
verifyUniqueRegion(addresses[10], 5, 5, 5);
verifyDirtiestRegion(addresses[10], 5, 5);
freeRun(addresses[0 .. 2]);
assert(regionAllocator.dirtyBlockCount == 11);
verifyUniqueRegion(addresses[0], 2, 2, 2);
verifyDirtiestRegion(addresses[10], 5, 5);
Comment on lines +686 to +689
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the order changed 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.

Moved it so as to test the region at addresses[10] remaining the longest dirty region at the end.

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 get it.

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 needed a case where a region is freed, but the currently-dirtiest region does not change. The re-ordering gives this.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's generally better to add new tests cases for what we want to test, as it prove none of the existing things are broken. Nevertheless, I get it now.

That being said, it seems that every single call to verifyUniqueRegion is now followed by a call to verifyDirtiestRegion. Then why have two functions?


// Merge regions and confirm expected effect.
freeRun(addresses[8 .. 10]);
assert(regionAllocator.dirtyBlockCount == 13);
verifyUniqueRegion(addresses[4], 10, 11, 11);
verifyDirtiestRegion(addresses[4], 11, 11);
freeRun(addresses[2 .. 4]);
assert(regionAllocator.dirtyBlockCount == 15);
verifyUniqueRegion(addresses[0], 14, 15, 15);
verifyDirtiestRegion(addresses[0], 15, 15);
freeRun(addresses[15 .. 16]);
verifyUniqueRegion(addresses[0], 1, RefillBlockCount, 16);
verifyDirtiestRegion(addresses[0], RefillBlockCount, 16);

// Test dirt behaviour in acquire and release.
void* addr0;
assert(regionAllocator.acquire(&addr0, 5));
assert(addr0 is addresses[5]);
assert(regionAllocator.dirtyBlockCount == 10);
verifyUniqueRegion(addresses[6], 1, RefillBlockCount - 6, 10);
verifyDirtiestRegion(addresses[6], RefillBlockCount - 6, 10);
regionAllocator.release(addresses[0], 6);
assert(regionAllocator.dirtyBlockCount == 16);
verifyUniqueRegion(addresses[0], 1, RefillBlockCount, 16);
verifyDirtiestRegion(addresses[0], RefillBlockCount, 16);
}
Loading