Weird generation bug when using qfapi with c2me mod, not present with fabric api (with quilt loader or fabric loader)

[offbeat-stuff]

• World generation is different on quilted fabric api and fabric api, when used with c2me

• With fabric api (on quilt loader, with Simply Optimized (modrinth modpack))
• With quilted fabric api (on quilt loader, with Simply Optimized (modrinth modpack)) *World Spawn point for same seed

[LambdAurora]

I don't know if I even want to question what in hell happened here.

(I lied, I think I know what happened)

It looks like all the grass/dirt went missing! Which can only indicate one thing: surface rules. QSL has a unique API compared to Fabric API which is the surface rules API which allows to add and modify those. My guess is C2ME being the nightmare it is doesn't support that with its aggressive multithreading.

I don't think it's for us to fix.

[HyperSoop]

Shouldn't QFAPI keep parity with FAPI though? Is there a reason behind this uniqueness?

[LambdAurora]

Please re-read my comment above: QSL has an API allowing for modification of surface rules. C2ME being a multithreading nightmare breaks such API.

[HyperSoop]

It might be a misconception on my end, but isn't QSL/QFAPI supposed to work with mods intended for Fabric just as FAPI would

[PhantomG27249]

The bug has been narrowed to an allocation optimization in c2me that is incompatible with the surface API. It has nothing to do with threading fortunately but I guess the surface API doesn't play nice with this one.

Mixin in question: https://github.com/RelativityMC/C2ME-fabric/blob/ver/1.19.3/c2me-opts-allocs/src/main/java/com/ishland/c2me/opts/allocs/mixin/surfacebuilder/MixinMaterialRulesSequenceMaterialRule.java

The easiest fix would probably be to probably disable the opt when loading with qfapi, alternatively, it might be possible for us to cache the surface API rules or something along those lines.

[LambdAurora]

Or alternatively we can investigate such optimization on our side and look if it can be implemented neatly in sync with the API, making breakability lower (if the same optimization is disabled in C2ME), could also reduce maintainability cost of the compatibility

[PhantomG27249]

If you guys want to implement such an optimization that would be great tbh because it would mean c2me doesn't lose perf when running with quilt and also makes compatibility less of a headache.

[LambdAurora]

I have to wonder, has any measuring been done on this code to measure the performance improvements?

[PhantomG27249]

I'll check with ishland when they are back this weekend, that opt has been there for a while and worked fine with qsl 1.19.2. A lot of the caching opts in c2me are to reduce the allocation rate of world gen which becomes somewhat unmanageable when threading becomes involved otherwise. Though tbh world gen in general benefits from allocation reductions so non thread cases might see gains too. Usually though allocation optimization is done based on profiling with jprofiler

[LambdAurora]

Yeah, talking to one of our worldgen expert and we're actually quite confused by the optimization:

Mojang has something in the lines of

SurfaceRules.SurfaceRule apply(SurfaceRules.Context context) {
	if (this.sequence.size() == 1) {
		return this.sequence.get(0).apply(context);
	} else {
		var builder = ImmutableList.<SurfaceRule>builder();
		
		for (var rule : this.sequence) {
			builder.add(rule.apply(context));
		}
		
		return new SequenceRule(builder.build());
	}
}

So we can see this as this diff:

SurfaceRules.SurfaceRule apply(SurfaceRules.Context context) {
-	if (this.sequence.size() == 1) {
-		return this.sequence.get(0).apply(context);
+	if (this.isSingleOrNoElement) {
+		return this.firstElement != null ? this.firstElement.apply(context) : EMPTY;
	} else {
-		var builder = ImmutableList.<SurfaceRule>builder();
+		var builder = ImmutableList.<SurfaceRule>builderWithExpectedSize(this.sequenceArray.length);
		
-		for (var rule : this.sequence) { // Iterator allocation.
+		for (var rule : this.sequenceArray) { // Compiles to iteration using an increment local.
			builder.add(rule.apply(context));
		}
		
		return new SequenceRule(builder.build());
	}
}

It is notable that checking for an empty list doesn't make much sense: the overload method explicitly disallows for this to happen and should be treat as a pre-condition of the record. Though it's what's causing this conflict to not actually crash and just generate a strange wasteland of stone.

The difference also explains why the conflict happens: you instantiate an array at the instantiation of the class and copy the content of the list, except we take a different approach: we create a root sequence rule that is populated after its creation, causing this bug in the first place. This detail is, iirc, very needed for the API to work properly and have a simple and yet powerful implementation in 1.19.2. While in 1.19.3 this got simplified a lot and does not require post modification, we probably could ditch this implementation detail entirely.

I'm quite interested in the builderWithExpectedSize tbh, it sounds very interesting and might be worth looking into ourselves? As for the array vs iterator, is something I definitely would like to get some data on.

[duplexsystem]

This issue has been worked around on the C2ME side.

[ishland]

Some raw benchmark data regarding list iteration vs array iteration and builder vs builderWithExpectedSize: https://gist.github.com/ishland/9b0a5b6f29727c8f2b460d377bacf9d6

[LambdAurora]

Wow, I was expecting something in those lines, glad to see I guessed it right!

1.19.3 definitely can use the array iteration optimization in theory, though I'm still slightly scared of the possibility of some people passing a mutable list. And for the builder one every version of QSL with this API can implement it.

Will look into it when I have time, thank you for the benchmarks!