Skip to content

Avoid duplicate objects for the same refmap - #242

Open
Nischhelm wants to merge 1 commit into
FabricMC:mainfrom
Nischhelm:refmap-cache-fabric
Open

Nischhelm wants to merge 1 commit into
FabricMC:mainfrom
Nischhelm:refmap-cache-fabric

Conversation

@Nischhelm

Copy link
Copy Markdown

Also PR'd upstream as SpongePowered/Mixin#719.

Every config parses its refmap again, even if another config already parsed the same resource, and keeps that copy for the whole session. Mods that split their mixins into one config per feature end up with one full copy of the same refmap per config.

A test Forge 1.12.2 instance (336 configs in both runs) gave:

Without this PR:
336 parsed refmaps, 95.1 MB of heap used, time spent parsing duplicates ~300ms

With this PR:
4 parsed refmaps, 0.8 MB of heap used, no extra time

Changes:

  • ReferenceMapper.readShared(String) caches successfully parsed refmaps by resource path and returns a new lightweight ReferenceMapper per call that shares the parsed mappings but has its own context.
  • Failed/missing refmaps are not cached, RemappingReferenceMapper and refMapperWrapper instances are still created per config.
  • Kept the TreeMaps from Make refmap json reproducible #100, they're now created in the constructor instead of at the field declaration.

ReferenceMapper objects hold some per-config state (the remapping
context), but mostly the mappings parsed from the refmap json. Since
multiple mixin jsons can refer to the same refmap json, parsing it once
per config wastes some time, but more importantly a lot of heap space.
As an example: a mod with 100 mixin jsons (one per feature) sharing one
refmap keeps 100 parsed copies of it, wasting e.g. about 30 MB of heap
for a 120 KB refmap. These copies stay on the heap for the entire
runtime.

To fix this, parsed refmaps are now cached by resource path, and each
config gets its own ReferenceMapper created by a second constructor,
which shares the already parsed mappings instead of parsing them again.

Other approaches to split by feature exist (IMixinConfigPlugin),
but not everyone uses them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant