Fix HXCPP_GC_BIG_BLOCKS row count regression

[dimensionscape]

#1402 changed mUsedRows and mFraggedRows in BlockDataInfo from int to uint8_t. That's enough for the default 32k blocks, which have 254 usable lines, but HXCPP_GC_BIG_BLOCKS uses 64k blocks with 508 usable lines, so the counts wrap once a block has more than 255 lines in use.

The worst effect is that the GC badly underestimates how much memory is in use after a collection. The working-memory target it sets stays far too small, so it ends up collecting almost every time it needs a new block, and a big-blocks build with a reasonable amount of live data spends most of its time in the GC. The wrap also means countRows() never sees a full block as full, and a block with exactly 256 used lines looks empty.

This changes both fields back to uint16_t, along with the allocator's mFraggedRows pointer and the casts into it. It also adds static_asserts so the counts can't end up smaller than IMMIX_USEFUL_LINES again. The byte counts are fine as uint16_t (508 x 128 = 65024).

See Repro:

Build with -D HXCPP_GC_BIG_BLOCKS and compare against a build without it. With the define, the churn loop is dramatically slower.

class Main {
	static function main() {
		var t0 = haxe.Timer.stamp();
		var live = new Array<Node>();
		for (i in 0...6000000)
			live.push(new Node(i));
		var t1 = haxe.Timer.stamp();
		var sum = 0;
		for (i in 0...30000000)
			sum += new Node(i).v & 1;
		var t2 = haxe.Timer.stamp();
		Sys.println('live set ${Std.int((t1 - t0) * 1000)}ms, churn ${Std.int((t2 - t1) * 1000)}ms ($sum, ${live.length})');
	}
}

class Node {
	public var v:Int;
	public var next:Node;

	public function new(v:Int)
		this.v = v;
}

Note: on current master you may also hit the marking crash fixed by #1407 while testing this.

[tobil4sk]

This comment doesn't seem very useful to keep, since it refers to the uint8_t that is removed by this PR. I think the static assert is more helpful at documenting the requirement

[dimensionscape]

Yeah, that's fair. It was originally a warning against regression, but the static asserts keep us safe. I'll remove it.

[Aidan63]

I think widening to uint16_t is the wrong move, we should define some BlockRow type which is either uint8_t or uint16_t depending on the big blocks define (see existing BlockIdType).
That way in combination with the sized constexpr I just pushed we can still have compile time errors for narrowing conversions and run time sanitisers will still be useful for non big blocks compiles.

[dimensionscape]

I think widening to uint16_t is the wrong move, we should define some BlockRow type which is either uint8_t or uint16_t depending on the big blocks define (see existing BlockIdType). That way in combination with the sized constexpr I just pushed we can still have compile time errors for narrowing conversions and run time sanitisers will still be useful for non big blocks compiles.

I’d just keep it as a single uint16_t.

The regression in #1402 came from using a type that was correct for default blocks but wrong for big blocks, and nobody caught it because big blocks isn’t built in CI (which highlights another issue). Then #1413 broke big blocks compilation and that went unnoticed for the same reason.

Making the row type depend on the define just keeps that blind spot and makes things more complex than they need to be. Some stray uint8_t row count such as a local, a cast, a helper parameter, or whatever will compile fine in the default build and only truncate with big blocks (which is not being run). With everything uint16_t, any accidental narrowing shows up as a conversion warning in the builds people actually compile, and the static asserts make it impossible to shrink the fields below IMMIX_USEFUL_LINES again.

Sanitisers don’t really change that. A default build never has more than 254 rows, so a uint8_t there won’t truncate in a correct program. The overflow that actually happened was 508 rows going into a uint8_t, and that case only exists with big blocks, where both versions are uint16_t anyway.

There’s also basically no practical downside to uint16_t. The fields get promoted to int in expressions anyway, so there’s no meaningful performance difference. The size cost is at most a few bytes of padding in a structure that already contains a ~500-byte hole table, once per 32 KB block. And the churn repro behaves the same in default builds either way...

[tobil4sk]

A default build never has more than 254 rows, so a uint8_t there won’t truncate in a correct program.

The reason you would want to use a sanitizer on the gc is when there is a bug in the gc, not in the user's program. There was a recent regression where mFraggedRows became corrupted: #1392. I'm not sure if this exact case would have been caught by uint8_t + sanitizer, but it should prevent some similar cases if there is a future regression.

big blocks isn’t built in CI (which highlights another issue)

We should solve the issue by adding a HXCPP_GC_BIG_BLOCKS build to the gc tests, rather than by trying to adapt the regular build to this gap in the tests.

[dimensionscape]

Agreed that the point of sanitising the GC is catching GC bugs, so let’s look at that directly. #1392 actually cuts the other way.

In #1392 a negative delta wrapped through an unsigned subtraction. With the field narrowed and the update written as it is now (*mFraggedRows += static_cast((spaceEnd - spaceStart) >> IMMIX_LINE_BITS)), that delta reaches the field as 2^N - k whatever the width N. The static_cast is explicit, so no sanitiser looks at it, and the += overflows the field in the same cases at 8 bits as at 16. A truncation check sees the same thing either way.

The difference is what’s left afterwards. A wrapped uint8_t lands in or next to the valid range (255, 254, 253...), so the corruption looks like a plausible count. A wrapped uint16_t lands at 65535/65534, which any invariant check flags immediately.

That’s the tool I’d reach for: a debug assert that mUsedRows and mFraggedRows stay within IMMIX_USEFUL_LINES. It catches corrupted counts in every debug build, at both block sizes, with no sanitiser build needed, and it works best with the wider type. hxcpp doesn’t run sanitisers in CI (the only toolchain switch is HXCPP_THREAD_SANITIZE, on mac), and narrowing isn’t UB, so it only shows up under clang’s opt-in implicit-conversion checks. Happy to add those asserts here.

The compile-time side works the same way. Narrowing diagnostics only fire where the source type is wider than the destination. With a define-dependent row type, the build everyone compiles has the narrowest field, so something like uint8_t rows{ info->mUsedRows } (the same kind of mistake as #1402) compiles cleanly by default and is only rejected in the big-blocks build. With one uint16_t it’s rejected in every build.

Agreed on CI: a HXCPP_GC_BIG_BLOCKS build belongs in the tests, and I’m happy to add it. It’s needed right now, since #1413 stopped big blocks from compiling at all (IMMIX_BLOCK_SIZE and IMMIX_USEFUL_LINES narrow in their brace initialisers). But a single type isn’t adapting the regular build to the gap. It’s the one type that’s correct in both configurations, so both builds exercise the same code.

The cost of uint16_t is 8 bytes per block: sizeof(BlockDataInfo) goes from 1568 to 1576 on x64, about 0.02% of a 32 KB block, and there’s no runtime difference.

[tobil4sk]

The concern for me is not about memory size or efficiency, it is about being accurate with the type used for what values are actually allowed to be represented. The point of #1402 was to be more accurate about this, but this seems to be a step backwards because we are adding back a large chunk of invalid state space.

The static_cast is explicit, so no sanitiser looks at it, and the += overflows the field in the same cases at 8 bits as at 16.

A large size_t over the signed 32 bit integer limit would also overflow a positive int which was used here originally, but that is not a good argument to use a wider type. Maybe for this bug it wouldn't matter if it is uint16_t, but any overflow beyond uint8_t is unexpected for the default build. A sanitizer is more effective at catching an overflow than manually placed debug asserts which might become stale or get missed somewhere.

[dimensionscape]

Alright, well neither type matches the valid range. Default blocks allow 0-254, so uint8_t still allows 255. Big blocks allow 0-508, so with either approach the field is a uint16_t with 65,027 invalid values. The real invariant is “at most IMMIX_USEFUL_LINES”, so I’ve added a check for that.

In debug builds, getStats(), countRows() and reclaim() now trap if either count is above IMMIX_USEFUL_LINES. Every block goes through one of them on each collection, so the check doesn’t rely on covering each write path, including future ones. It also catches cases a sanitiser can’t: 255 fits in a uint8_t, and both fragged-row updates are explicit static_casts, which sanitisers skip.

I’ve also added the big-blocks build to CI as you suggested. It’s a debug build of test/haxe, which includes the GC tests, so the checks run on every PR and can’t go stale. That build needed IMMIX_BLOCK_SIZE and IMMIX_USEFUL_LINES widened (65536 and 508 with big blocks), since #1413’s brace initialisers don’t compile there, so I merged master and fixed those too..

[Aidan63]

tobil4sk is right that the benefit is being able to tell what an acceptable range for variables are, I have zero concerns about the size / memory / performance difference for uint16 vs uint8. I knew that the constexpr change broke more stuff but I was hoping we would quickly fix it as part of this PR (probably shouldn't have pushed to master anyway).
Since that doesn't seem to be happening I made what I think is a better fix this morning. I'll try to find some time later this evening to give it a once over and put it up for merge, that should then get things working again and supersede these changes.

[dimensionscape]

Okay, well, the #1413 fix is already in this PR, and CI passes. The only failure is Windows (64bit) (latest) / cffi(which also fails on master). The static_asserts and debug check cover the actual limit. It’s ready to merge as is. If you’d rather go with your own fix, that’s your call.