Track dirty pages in regionAllocator. #338

dsm9000 · 2023-11-06T16:10:47Z

Cut-down split of #335 : only the dirt tracking per se.

sdlib/d/gc/region.d

deadalnix · 2023-11-06T21:48:24Z

There is no test that the dirty pages are actually tracked properly. Making sure it is in all circumstance, in a data structure that make sense, is a big enough challenge, no reason to add more. However, it's a good reason to make sure this is well tested and correct in all circumstance, so it can serve as a layer fancier things can be built upon.

dsm9000 · 2023-11-06T21:54:30Z

@deadalnix The re-using logic and its test, were the bulk of the test for this. The remainder is e.g. here. What kind of test do you think it would be appropriate to add here while maintaining the given split?

deadalnix · 2023-11-06T23:00:16Z

With all the scenari that can happen and need to be kept track off, I'm confident that if this is mixed with reusing, the testing is not appropriate. Thankfully, if the PR is made complicated enough, this fact can be obfuscated.

dsm9000 · 2023-11-07T00:33:09Z

@deadalnix are you able to point to something concrete and say "this part is needlessly complicated, it is possible to fulfill the spec without it" or is your objection simply "too much green in this diff" ?

sdlib/d/gc/region.d

deadalnix · 2023-11-07T18:20:59Z

sdlib/d/gc/region.d

-
-		auto r0 = Region.fromSlot(slot, 0);
-		auto r1 = Region.fromSlot(slot, 1);
+		static assert(Region.sizeof <= ExtentSize, "Unexpected Region size!");


Isn't there a way to make it fit? It seems wasteful to multiply by two the amount of space taken by the metadata to store one size.

Fits, naturally, if the by-dirt tree is cut out. But not otherwise.

We'll ask ourselves how to fit the data in there when we actually run out of space.

Is it possible to run out of metadata slots other than under total OOM?

sdlib/d/gc/region.d

deadalnix · 2023-11-07T18:28:32Z

sdlib/d/gc/region.d

@@ -144,6 +178,7 @@ private:

 			r.merge(adjacent);


The merging here does not keep track of dirty pages, which was the whole point of this PR.

It does though, see the Region.merge()

No. Region.merge asserts a few things as it assume they are true, when they clearly do not necessarily are, which would have been obvious in the presence of tests.

Why do I need to runt he code through my mind to figure this out? If I'm wrong, it's trivial to demonstrate with a test that checks for the various permutations of things.

The Region.merge() in this split is provably correct (and I'll add the test) while it remains the case that we don't purge dirties. But in order to be correct without this assumption, it needs the ordering function from part3 (and see its test.) Do you want these in part1? It seemed redundant IMHO to have them without the purging.

I think you are correct, but you got to admit this wasn't obvious when this was drowned in a sea of unrelated changes.

My goal is at all times to make things obviously correct to the reviewer. When failed, i.e. it it isn't obvious -- then needs rework, I won't disagree.

deadalnix · 2023-11-07T18:29:50Z

There is a ton of code that has nothing to do with what this PR tries to achieve. And there is code that clearly lost track of the dirty pages in there.

Which also means that the testing for all of this is inadequate, as tests shouldn't be passing when the code is doing the wrong thing.

sdlib/d/gc/region.d

deadalnix · 2023-11-07T19:12:07Z

sdlib/d/gc/region.d

 	Region* merge(Region* r) {
-		assert(address is (r.address + r.size) || r.address is (address + size),
-		       "Regions are not adjacent!");
+		auto rIsLeft = address is (r.address + r.size);
+		auto rIsRight = r.address is (address + size);
+		assert(rIsLeft || rIsRight, "Regions are not adjacent!");
+
+		auto leftRegion = rIsLeft ? r : &this;
+		auto rightRegion = rIsRight ? r : &this;
+
+		// Dirt is at all times contiguous within a region, and starts at the bottom.
+		// Given as purging is not yet supported, this invariant always holds.
+		assert(!leftRegion.hasClean || !rightRegion.hasDirt,
+		       "Merge would place dirty pages in front of clean pages !");
+
+		return at(leftRegion.address, size + r.size, dirtySize + r.dirtySize);
+	}
+
+	void setDirt(size_t newDirtySize) {
+		dirtySize = newDirtySize;
+	}
+
+	@property
+	bool hasClean() {
+		return dirtySize < size;
+	}

-		import d.gc.util;
-		auto a = min(address, r.address);
-		return at(a, size + r.size);
+	@property
+	bool hasDirt() {
+		return dirtySize > 0;
 	}


Region* merge(Region* r) { assert(address is (r.address + r.size) || r.address is (address + size), "Regions are not adjacent!"); auto left = address < r.address ? &this : r; auto right = address < r.address ? r : &this; // Dirt is at all times contiguous within a region, and starts at the bottom. // Given as purging is not yet supported, this invariant always holds. assert(left.dirtySize == left.size || right.dirtySize == 0, "Merge would place dirty pages in front of clean pages !"); import d.gc.util; auto a = min(address, r.address); return at(a, size + r.size, dirtySize + r.dirtySize); }

return at(left.address, size + r.size, dirtySize + r.dirtySize);

sdlib/d/gc/region.d

deadalnix · 2023-11-08T01:21:53Z

sdlib/d/gc/region.d

@@ -144,6 +178,7 @@ private:

 			r.merge(adjacent);


I think you are correct, but you got to admit this wasn't obvious when this was drowned in a sea of unrelated changes.

sdlib/d/gc/region.d

dsm9000 mentioned this pull request Nov 6, 2023

Track dirty pages in regionAllocator and preferentially reuse them when possible. #335

Closed

deadalnix reviewed Nov 6, 2023

View reviewed changes

sdlib/d/gc/region.d Outdated Show resolved Hide resolved