Reduce poi memory

[jcw780]

Reduce point of interest memory impact by not adding empty subchunks to the storage hashmap and by unloading non-portal searched points of interest.

Significant updates were made so extensive testing is likely required.

[2No2Name]

Does this PR only reduce memory usage, or did you also see any performance impact? I think we already talked about checking how lodestone compasses may be affected

[jcw780]

Does this PR only reduce memory usage, or did you also see any performance impact? I think we already talked about checking how lodestone compasses may be affected

Primary objective is to reduce memory impact - mitigate the point of interest memory leak. I did not notice significant mspt changes compared to existing benchmarks - would expect some small changes since the storage hashmap is now a lot smaller. Some effort was taken to reduce the effect of removing Optional.empty() in general use.

The new logic does seem to have significantly less unloading impact than a naive implementation because there are usually much fewer hashmap entries to remove. For instance, to unload a chunk without PoiSections it will only have to remove the Bitset from column vs that plus 16 or 24 - sections in chunk - Optional.empties() in the storage hashmap.

Stuff that constantly getOrLoad points of interest - like compasses inventoryTicked by LivingEntities - will have to frequently load the chunk to border or higher then unload it entirely - to empty - and for the chunk to not have been searched by portal ensureLoadedAndValid to observe persistent lag effects. Otherwise it will just be one read just after it gets unloaded.

There may be performance benefits if a server would otherwise be approaching memory limit without the optimizations - e.g. long running server where a lot of chunks were loaded at some point. But I have not tested that exact case.

[ishland]

Is it possible for this to be lazy-evaluated with something like Suppliers#memoize?

We have a optimization that calls read asynchronously, and this calls into non-threadsafe methods in POI manager, which is illegal in this context. Having this lazy-evaluated makes sure it goes into the checkConsistencyWithBlocks below, which is guarded with async-dispatch through @WrapOperation.

[2No2Name]

I can move this to the @Redirect mixin immediately below, initializing in the first loop iteration. Does the @Redirect need to be changed to a WrapOperation to be compatible with your mod?

[2No2Name]

The resulting code would be

    @Redirect(method = "read", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/entity/ai/village/poi/PoiManager;checkConsistencyWithBlocks(Lnet/minecraft/core/SectionPos;Lnet/minecraft/world/level/chunk/LevelChunkSection;)V"))
    private void redirectCheckConsistencyWithBlocks(PoiManager poiManager, SectionPos sectionPos,
                                                    LevelChunkSection levelChunkSection,
                                                    @Local(name = "pos", argsOnly = true) ChunkPos pos,
                                                    @Share("column") LocalRef<BitSet> column) {
        if (column.get() == null) {
            column.set(((RegionBasedStorageSectionExtended<?>) poiManager).lithium$getNonEmptyPOISections(pos.x(), pos.z()));
        }
        ((PoiCheckConsistency<PoiSection>) poiManager).lithium$CheckConsistencyWithBlocks(sectionPos, levelChunkSection, column.get());
    }

[ishland]

The resulting code would be

    @Redirect(method = "read", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/entity/ai/village/poi/PoiManager;checkConsistencyWithBlocks(Lnet/minecraft/core/SectionPos;Lnet/minecraft/world/level/chunk/LevelChunkSection;)V"))
    private void redirectCheckConsistencyWithBlocks(PoiManager poiManager, SectionPos sectionPos,
                                                    LevelChunkSection levelChunkSection,
                                                    @Local(name = "pos", argsOnly = true) ChunkPos pos,
                                                    @Share("column") LocalRef<BitSet> column) {
        if (column.get() == null) {
            column.set(((RegionBasedStorageSectionExtended<?>) poiManager).lithium$getNonEmptyPOISections(pos.x(), pos.z()));
        }
        ((PoiCheckConsistency<PoiSection>) poiManager).lithium$CheckConsistencyWithBlocks(sectionPos, levelChunkSection, column.get());
    }

That code change should work to my knowledge.

[2No2Name]

Are we sure that the map does not contain a value already for this key?

If not, this should include calling instance.remove

[jcw780]

unpackChunk callers check that the chunk is not already loaded [in the loadedChunks hashSet] before running unpackChunks. "POI Chunks" - and therefore poi sections - should not exist from before.

[jcw780]

It's a bit complicated than that, because of pendingLoads but that appears to also be checked for. i.e. a new future is only created if it's not in pendingLoads as well.

[2No2Name]

I am not sure how exactly the pending loads work, it seems like the check you are referring to is outside the CompletableFuture. It looks like that pending loads may take multiple chunk map ticks to complete, this does not feel correct.

[jcw780]

The other call for unpackChunk will computeIfAbsent a future then join it if it does not exist in the pendingLoads hashmap. So even if it does not complete in time, if another call needs it now then it will not create another future and the entry will be removed from pendingLoads upon completion.

[2No2Name]

I pushed my changes here: https://github.com/CaffeineMC/lithium/tree/pr/reduce-poi-memory

[2No2Name]

Replies from discord

ensureloadedandvalid: I flipped the order of lowestSection < maxYSectionIndexExclusive && loadedChunks.add(ChunkPos.asLong(x, z) to loadedChunks.add(ChunkPos.pack(x, z)) && lowestSection < maxYSectionIndexExclusive so that loadedChunks always gets added to

lithium$CheckConsistencyWithBlocks: ngl I didn't really think of that, it's just something that shouldn't happen unless something [probably another mod] desynchronized the columns bitset and the sectionstorage storage hashmap