perf: resolve tracked resources in bulk when opening a Grid

[Klaas058]

perf(storage): resolve tracked resources for the whole network in one pass

Replaces #1405. Also replaces the now closed #1403.

Sorry for the size of this one. The idea is small and most of the diff is repetitive, so let me walk you through it. The detail is in collapsed sections to keep this readable.

Both #1403 and #1405 went after the symptom: the stream allocations inside findTrackedResourceByActorType. That made each lookup cheaper, but there were still just as many lookups. The allocations were the result of calling it millions of times, not the reason it was slow.

The change

Opening a Grid asked every storage about every resource. That is resources x storages, so 37,000 resources across 80 disks means 2,960,000 lookups.

Now every storage hands over what it knows once, into a single map.

Three new default methods. TrackedStorage and TrackedStorageRepository both get collectTrackedResourcesByActorType, which hands every tracked resource a storage knows about to a consumer. TrackedStorage also gets getTrackedResourcesByActorType, which collects those into one map. InMemoryTrackedStorageRepository is the only one that actually walks its map, everything else just forwards to its delegate.

Nothing breaks for addons. Both interfaces are @API(status = STABLE). Nothing existing changed, I only added default methods, so addons keep compiling and loading as they are. And because the default body is the old per-resource loop, an addon that does not override it still gets the right answer, it just does not get faster.

Results

37,000 resources, 80 disks:

Metric	develop	this PR	Ratio
Grid open	741.3 ms	92.0 ms	8.1x
resource resolution	661.3 ms	14.7 ms	45x
Grid open, allocated	1884.9 MiB	110.2 MiB	17.1x
resource resolution, allocated	1784.7 MiB	11.8 MiB	150.8x
leaf lookups	2,960,000	80	37,000x

The last row is the one that matters, the rest follows from it. It is also the only row not from spark, because a sampling profiler cannot count calls. That number comes from the JVM benchmark further down, which counts them directly. The other four rows are spark averages over three runs each.

Full profiles, scaling data and the microbenchmark

Scaling confirms the cause

Three setups, each captured three times on the spark execution profiler and three times on the allocation profiler. All 18 captures are linked below. open is the whole Grid open, getResources is the part that resolves the resources, find is findTrackedResourceByActorType.

Spark samples every 4 ms, so the execution numbers only come in steps of 4.

Execution, ms:

Configuration	Run	open	getResources	find	collect
develop, 16 disks	1	224.0	144.0	144.0	n/a
develop, 16 disks	2	240.0	160.0	156.0	n/a
develop, 16 disks	3	252.0	156.0	156.0	n/a
mean		238.7	153.3	152.0
develop, 80 disks	1	704.0	632.0	632.0	n/a
develop, 80 disks	2	748.0	664.0	664.0	n/a
develop, 80 disks	3	772.0	688.0	688.0	n/a
mean		741.3	661.3	661.3
this PR, 80 disks	1	96.0	20.0	0.0	0.0
this PR, 80 disks	2	92.0	12.0	0.0	0.0
this PR, 80 disks	3	88.0	12.0	0.0	0.0
mean		92.0	14.7	0.0	0.0

Allocation, MiB:

Configuration	Run	open	getResources	find	collect
develop, 16 disks	1	527.1	426.0	415.0	n/a
develop, 16 disks	2	536.6	434.0	422.0	n/a
develop, 16 disks	3	567.8	467.5	459.0	n/a
mean		543.8	442.5	432.0
develop, 80 disks	1	1923.8	1826.5	1817.5	n/a
develop, 80 disks	2	1853.8	1744.0	1737.5	n/a
develop, 80 disks	3	1877.1	1783.5	1775.5	n/a
mean		1884.9	1784.7	1776.8
this PR, 80 disks	1	111.8	14.0	0.0	1.0
this PR, 80 disks	2	106.1	9.0	0.0	0.5
this PR, 80 disks	3	112.8	12.5	0.0	0.5
mean		110.2	11.8	0.0	0.7

Going from 16 to 80 disks on develop is 5x the storages, with the same number of resources. find gets 4.35x slower and allocates 4.11x more. Cost grows with the number of storages, which is the whole problem.

This PR at 80 disks is still better than develop at 16: resolution is 10.5x faster and allocates 37.4x less.

The biggest allocator on develop is StreamSupport.stream at 980.5 MiB, which is the stream #1405 tried to remove. After this change the top allocators are netty buffers, so just sending the packet.

Grid open is still 92 ms, and only 14.7 ms of that is resource resolution. The other 77 ms is menu construction and packet serialization, which this PR does not touch.

Microbenchmark

A plain JVM benchmark that does the same work outside the game, 5 warmup and 10 measured runs, counting lookups directly. It is not part of this PR, I removed it before submitting since it is not a test. The numbers are lower than in-game because there is no menu and no networking. Allocation lines up with spark (141x here, 150.8x in-game). Time looks better here (108x versus 45x) because spark's getResources also covers work this benchmark does not do.

Scenario	Resources	Sources	Mode	Replay ms	Allocated MB	Leaf lookups
small	500	16	per-resource	1.17	1.19	8000
small	500	16	enumerated	0.09	0.04	16
medium	2000	40	per-resource	5.53	10.08	80000
medium	2000	40	enumerated	0.27	0.14	40
large	10000	80	per-resource	47.34	95.41	800000
large	10000	80	enumerated	0.54	0.67	80
spark-repro	37000	80	per-resource	171.15	353.00	2960000
spark-repro	37000	80	enumerated	1.59	2.51	80
stale-heavy	1000	80	per-resource	4.14	9.54	80000
stale-heavy	1000	80	enumerated	1.38	1.68	80
stale-extreme	500	80	per-resource	2.41	4.77	40000
stale-extreme	500	80	enumerated	0.59	0.04	80
stale-brutal	200	80	per-resource	1.00	1.91	16000
stale-brutal	200	80	enumerated	0.10	0.02	80

The stale-* rows say "enumerated" because that is the method being called, but those are exactly the cases where the guard kicks in and does direct lookups instead.

On trusting these numbers

Flame graphs are easy to read wrong. A composite calling a composite calling a disk shows up at every level, so the same work can look like several times the time it actually took. I only counted the outermost occurrence of each frame, ran every setup three times, and checked the result against the lookup count, which sampling cannot get wrong. Every capture is linked above, so please do check rather than take my tables at face value. I can share the test world if that helps.

Why default methods, and the one subtlety

Since the default body is the old per-resource loop, there is nothing to opt into, no marker interface to check, and no way for an implementation that has not been updated to give a wrong answer. It is per storage too: 79 updated disks and one addon storage means 79 fast and one slow, not the whole network falling back.

Tracked resources are never cleaned up. A disk that has seen a million different items still holds a million entries, even if it currently holds ten. Walking that map blindly means doing work based on everything the disk has ever seen instead of what is on it now. Hence the guard in InMemoryTrackedStorageRepository:

if (resources.size() < tracked.size()) {
    // one map lookup per requested resource instead
}

I found this the hard way. Without the guard, the stale-brutal case (200 items stored, 1,000,000 tracked) took 63.56 ms and 46.53 MB, worse than develop's 1.00 ms. With it, 0.10 ms and 0.02 MB.

The new method may return more than you asked for. This is the contract of getTrackedResourcesByActorType itself, not a change to anything: findTrackedResourceByActorType still behaves exactly as it did, so no existing code sees a difference.

When a storage holds fewer tracked entries than the number of resources being asked about, it hands over all of them instead of checking each one, and some of those may have since been removed from the network. The javadoc says so. Our callers loop over what is actually stored and look each one up in the result, so the extra entries are never read. An addon that wants an exact result can pass a Set instead of a List and the repository will filter, which is free because contains on a Set is already O(1). Doing it for everyone would mean building a HashSet on each call, which is why it is not the default. Happy to change that if you would rather have it.

Ties resolve the same as before. The old code used Stream.max(comparingLong(getTime)), which keeps the first one on a tie. The new code uses HashMap.merge with existing.getTime() >= other.getTime() ? existing : other, which also keeps the first one. Both end up with the first storage holding the newest timestamp, nested composites included. Two tests cover this.

A pre-existing issue I did not touch

This is already on develop and has nothing to do with this PR, I just ran into it while measuring. ExternalStorageTrackedStorageRepository saves its entire tracking map, so external storages keep collecting entries forever. Disks do not have this problem because PlatformStorage.toContents() only saves what is actually stored, which clears the rest on save and load. The guard above stops that growing map from being a performance problem here, but the growth itself is a separate thing. Happy to open an issue for it.

Full transparency

I was assisted in this PR by AI (Claude fable 5.1 and Gemini 3.8). This is in no way intended as a slop PR, I'm a full time software engineer and reviewed every line of code. Tested it myself and calculated the results myself, I believe this is sound.

Happy to answer any questions

[SirYwell]

This looks promising. Besides some details we can discuss later, I have a few questions and remarks though:

1. Set#contains(Object) does not have any asymptotic guarantees. HashSet#contains(Object) has expected $O(1)$ behavior, but e.g., TreeSet#contains(Object) has $O(\log n)$ asymptotic complexity.
2. I also don't understand the point of accepting Collection: any Iterable would work, but what is the semantics of duplicate ResourceKeys?
3. From my understanding, the resources passed to the consumer might overapproximate the result. Given we collect that into a map, how much of a problem can that become worst-case? Even if we don't look up the value, it still consumes memory. Pre-sizing the map just to add more elements also might be a(n unnecessary?) problem.
4. The PR description says tracked resources are never cleaned up. That sounds like an orthogonal problem. Is that something that should be addressed in general or is that only a problem for this very specific situation?

[Klaas058]

Hi @SirYwell thanks, good questions

1. Set#contains(Object) does not have any asymptotic guarantee

You're right, I forgot about TreeSet. I will delete the claim. TreeSet does satisfy the Set contract and is O(log n)

2. Why Collection and not Iterable

The reason was mostly because I wanted to have the size to guard against point 4 and the getAll on the Storage already returns a Collection. Which also avoids a new allocation for a HashSet. Requiring a Set is actually cleaner and also avoids the possible duplicate keys too. Will update it.

3. The resources passed to the consumer might overapproximate & pre-sizing

Hmm yes I see what you mean, worst case would be sources x resources. This will also be fixed by typing resources as Set<ResourceKey>, newHashMap(resources.size()) can never rehash in that case because it only filters results from resources. It is still oversized in the common case, where few resources have a tracked entry for the requesting actor, so I am happy to drop the pre-size if you prefer

4. The PR description says tracked resources are never cleaned up. That sounds like an orthogonal problem

Yes, this is orthogonal and I think it is best addressed in a separate PR. From what I see: Disks themselves are pruned indirectly PlatformStorage.toContents() writes one entry per currently stored resource with its tracking data attached, so anything no longer stored is gone on save. External storages are not: ExternalStorageTrackedStorageRepository#getTrackedResources serializes its whole PlayerActor map with no check against current contents, and load restores it in full, so it grows across sessions without bound.

Reproduction, as a test in refinedstorage-common

package com.refinedmods.refinedstorage.common.storage.externalstorage;

import com.refinedmods.refinedstorage.api.core.Action;
import com.refinedmods.refinedstorage.api.storage.StorageImpl;
import com.refinedmods.refinedstorage.api.storage.tracked.TrackedStorageImpl;
import com.refinedmods.refinedstorage.common.MinecraftRegistriesTest;
import com.refinedmods.refinedstorage.common.RefinedStorageApiImpl;
import com.refinedmods.refinedstorage.common.api.RefinedStorageApi;
import com.refinedmods.refinedstorage.common.api.RefinedStorageApiProxy;
import com.refinedmods.refinedstorage.common.api.storage.PlayerActor;
import com.refinedmods.refinedstorage.common.support.resource.ItemResource;

import net.minecraft.world.item.Items;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;

@MinecraftRegistriesTest
class ExternalStorageTrackedStorageRepositoryTest {
    @BeforeAll
    static void setUpApi() {
        // Needed if you want to test this in isolation so it can be loaded without an injected API
        ((RefinedStorageApiProxy) RefinedStorageApi.INSTANCE).setDelegate(new RefinedStorageApiImpl());
    }

    @Test
    void shouldKeepTrackingResourceThatIsNoLongerStored() {
        // Arrange
        final ExternalStorageTrackedStorageRepository sut = new ExternalStorageTrackedStorageRepository(() -> {
        });
        final TrackedStorageImpl storage = new TrackedStorageImpl(new StorageImpl(), sut, () -> 1L);
        final ItemResource dirt = new ItemResource(Items.DIRT);

        // Act
        storage.insert(dirt, 1, Action.EXECUTE, new PlayerActor("A"));
        storage.extract(dirt, 1, Action.EXECUTE, new PlayerActor("A"));

        // Assert
        assertThat(storage.getAll()).isEmpty();
        assertThat(sut.getTrackedResources()).hasSize(1); // <-- Still tracked, even though nothing is stored
    }
}

getTrackedResources() is what AbstractExternalStorageBlockEntity saves and load is what it restores, so the entry survives every reload, and repeating. As long as the item is unique, the it keeps growing.

[Klaas058]

Re-ran the benchmark on the 80 disk world with resources typed as Set<ResourceKey> using the same method as before

CPU (ms, Server thread)

Run	Spark	tryOpenScreen	getResources	of which Collectors.toSet
Set, 80 disks #1	DtkDsi3hjT	160	28	12
Set, 80 disks #2	dJzQlygDoJ	108	20	16
Set, 80 disks #3	KT4mJLlLKY	100	20	12
mean		122.7	22.7	13.3
previous patched (List), mean		92.0	14.7	n/a
develop, mean		741.3	661.3	n/a

Allocation (MiB, Server thread)

Run	Spark	Thread total	tryOpenScreen	getResources	of which Collectors.toSet
Set, 80 disks #1	FbJJO7KfmE	111.3	96.3	16.0	5.0
Set, 80 disks #2	0H4jlHFju5	123.6	113.6	16.5	7.0
Set, 80 disks #3	FluRgsIpLU	120.8	108.8	21.0	7.0
mean		118.6	106.2	17.8	6.3
previous patched (List), mean		124.9	110.3	11.8	n/a
develop, mean		1,898.6	1,884.9	1,784.7	n/a

This result is not suprising as the Collectors.toSet() builds a new set from the entire network. On a network with 37k unique items that is significant. This could be avoidable though because the ResourceList.getAll() already returns Set<ResourceKey>, it is just not reachable through RootStorage, which only exposes Collection<ResourceAmount> getAll(). It means a new method on a STABLE interface, so I have not done it here

[SirYwell]

I'm glad always using Set performs similar, that's a good sign. The toSet() is not that much more costly than a toList() (the data is in the noise there). I'll take a closer look at the changes tomorrow.