Finalization support for large allocs. #310

dsm9000 · 2023-10-12T19:02:05Z

Note: the extent side support split into #311

sdlib/d/gc/tcache.d

deadalnix · 2023-10-12T19:38:06Z

sdlib/d/gc/tcache.d

+
+unittest finalization {
+	// Faux destructor which simply records most recent kill:
+	static size_t lastKilledUsedCap = 0;


A cap is how many things you do before you stop and/or saturate.

sdlib/d/gc/tcache.d

deadalnix · 2023-10-12T19:55:09Z

sdlib/d/gc/tcache.d

+	void freeImpl(PageDescriptor pd, void* ptr) {
+		pd.arena.free(emap, pd, ptr);
+	}
+
+	void destroyImpl(PageDescriptor pd, void* ptr) {
+		auto e = pd.extent;
+		if (e is null) {
+			return;
+		}
+
+		if (pd.isSlab()) {
+			// Slab is not yet supported
+			return;
+		}
+
+		if (e.finalizer is null) {
+			return;
+		}
+
+		(cast(void function(void* body, size_t size)) e.finalizer)(
+			ptr, e.usedCapacity);
+	}


I don't understand why this is destroyImpl rather than destroy itself. It has one callsite. Is it a generally useful abstraction?

@deadalnix it lets us avoid having to find the pd twice (destroy and then in free)

I still don't get why this isn't in the destroy method. What is the point of having two methods?

deadalnix · 2023-10-12T19:56:08Z

sdlib/d/gc/tcache.d

@@ -347,6 +377,29 @@ private:
 		return true;
 	}

+	void freeImpl(PageDescriptor pd, void* ptr) {
+		pd.arena.free(emap, pd, ptr);


I'm not sure this is such a great idea, as it hides the fact this is using the emap. But the emap need to be initialized before it is used.

deadalnix · 2023-10-12T20:15:32Z

sdlib/d/gc/tcache.d

@@ -127,6 +173,7 @@ public:
 		if (isLargeSize(size)) {
 			auto npd = getPageDescriptor(newPtr);
 			npd.extent.setUsedCapacity(size);
+			npd.extent.setFinalizer(finalizer);


Don't do finalization magic in realloc. this is not necessary at this time, and this is untested anyways.

deadalnix · 2023-10-12T20:17:21Z

sdlib/d/gc/tcache.d

+		freeImpl(pd, ptr);
+	}
+
+	bool setFinalizer(void* ptr, void* finalizer) {


You can't add a finalizer ex post facto because you don't know you have the room (you have it for large, but you can't know that from ptr).

You want to allocate with a finalizer.

https://github.com/snazzy-d/sdc/pull/311/files#diff-14fa859f3f1ce9782b36cdb23977f43bbae0b791fac441958f71ca167c5f52fbR403 is how I propose to handle it. idea is that we can set finalizer on any small allocs which are appendable. we'll have a allocFinalizable which calls allocAppendable and then sets the finalizer.

sdlib/d/gc/tcache.d

deadalnix · 2023-10-12T20:48:54Z

sdlib/d/gc/tcache.d

+	static size_t lastKilledUsedCapacity = 0;
+	static void* lastKilledAddress;
+	static uint destroyCount = 0;
+	static void destruct(void* body, size_t size) {


body used to be a keyword, and it poorly highlighted. ptr is better, this what it is everywhere else anyway.

deadalnix · 2023-10-12T22:04:50Z

sdlib/d/gc/tcache.d

+
+		auto e = pd.extent;
+		// Slab is not yet supported
+		if (e is null || pd.isSlab() || e.finalizer is null) {


Invert the condition so you don't need to call the same function twice.

deadalnix · 2023-10-12T22:14:42Z

sdlib/d/gc/tcache.d

+			return;
+		}
+
+		(cast(void function(void* ptr, size_t size)) e.finalizer)(


Shouldn't the accessor do the cast?

Tried this earlier; any attempt to call a function pointer without a cast, e.g.

alias Finalizer = void function(void* ptr, size_t size);

e.finalizer(ptr, e.usedCapacity); in destroy(),

void* allocAppendable(size_t size, bool containsPointers, Finalizer finalizer = null)
etc.

... and using this as type for wherever finalizers are seen, bombs with core.exception.AssertError@src/d/common/type.d(375): Not a function. on sdc, with kilometres of llvm soup.

Edit: does work if not using alias.

deadalnix · 2023-10-13T12:47:38Z

sdlib/d/gc/extent.d

+	@property
+	void function(void* ptr, size_t size) finalizer() {
+		assert(isLarge(), "Finalizer accessed on non large!");
+		return cast(void function(void* ptr, size_t size))
+			_metadata.largeData.finalizer;
+	}
+
+	void setFinalizer(void* finalizer) {
+		assert(isLarge(), "Cannot set finalizer on a slab alloc!");
+		_metadata.largeData.finalizer = finalizer;
+	}


I don't understand what's up here.

It's like you put one foot forward and then don't realize that the second foot can move forward too.

If finalizer gives you a function, then setFinalizer should accept a fucntion, and the field itself almost certainly would benefit from being a function.

…gealloc_only

deadalnix · 2023-10-13T16:00:55Z

sdlib/d/gc/tcache.d

@@ -1,5 +1,6 @@
 module d.gc.tcache;

+import d.gc.extent;


This for one alias? A strong sign the alias isn't defined where it should be.

dsm9000 added 3 commits October 12, 2023 14:59

finalization support for large allocs.

c183294

finalization support for large allocs.

7f1fc9e

finalization support for large allocs. (corrected)

07272ec