Repository navigation
Cut the join pack reload from 8 s to 3 s - #50
Merged
Merged
Conversation
Merge ItemsAdder's one-glyph font providers into grid sheets and split the core pack into three parts by namespace. The client checks every font provider for every code point it knows, including unifont's ~60,000, so 616 one-glyph providers froze the render thread for over 2 s. Same-shaped glyphs now share a sheet (616 -> 309 on Main; the rest use vanilla textures the server does not have). A 1.21 client reads every entry of a zip once per namespace it holds each time it lists resources, and ZipFile is synchronised. Core held 33 namespaces and 19,795 entries; splitting it into three parts that minimise namespaces x entries cuts that work about 5x and lets the workers read in parallel. On a vanilla 1.21.10 client with Main's pack, a warm reload drops from 8.0 s to 3.0 s and the render-thread block from 2.7 s to 1.1 s. Every glyph cell, height, ascent and filter is unchanged, the client logs the same warnings, and models.zip and sounds.zip keep their hashes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
the
size:L
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
- Keep generations under parts-v2: an existing three-part generation of the same ItemsAdder hash failed the integrity check and blocked publishing. - Bound font and texture reads as they inflate (protected ZIPs mask sizes), cap each glyph at 1024x1024 and the glyphs held at 16M pixels. - Reject what stb_image rejects: unknown critical chunks, IHDR not first or repeated, PLTE/tRNS after IDAT or repeated, tRNS before PLTE, bad palettes and non-standard header fields. Such glyphs stay unmerged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Check a glyph's pixel count against its own cap and the build's remaining budget from IHDR, before anything is inflated or allocated. - Reject what stb_image rejects: a bad zlib header (check bits, preset dictionary, not deflate), a stream with no final block, no IEND, an empty palette, and critical chunks by the PNG bit rather than letter case. - Reject palette indices past the palette: stb reads past it, ImageIO reads black, so a merged copy could differ. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Palette chunks only on palette images, before the data; no tRNS on RGB or RGBA; image data as one run of non-empty IDAT chunks with the palette already present; the four Adler-32 bytes present after the stream (their value is still not checked). Anything else stays unmerged. - Hold at most 16 MiB of rewritten fonts and sheets; later fonts stay as ItemsAdder wrote them. Against LWJGL/stb 3.3.3 on a 7,026-image corpus, every image the merger accepts now decodes to the same pixels. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 16 MiB output check ran only after a sheet was built and encoded, so a 4096x4096 sheet of incompressible glyphs could exhaust a small heap first. Sheets are now at most 2048x2048, glyphs at most 512x512, and a font's glyphs at most 4M pixels (images are freed after each font). Each sheet is encoded into a stream capped at the output room left; a font that outgrows it stays as ItemsAdder wrote it. Main's largest glyph is 256x256 and its busiest font 1.77M pixels, so its parts are byte-identical to before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Reviewed by GPT-6.1-Sol (xhigh) over five rounds; the final round's verdict was OK with no findings. It checked decoding against LWJGL/stb 3.3.3 on 41,982 PNGs (zero mismatches), bounded memory under -Xmx256m, deterministic output on Main's real pack, and the parts-v2 upgrade. CodeRabbit is rate-limited, so this is merged with --admin. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Joining Main spends 11–16 s on the pack even when it is cached. I profiled the reload on a real client (per-step reload timing plus Java Flight Recorder) and found two causes, both in how the pack is laid out.
Fonts. The client checks every provider of a font for every code point it knows, including unifont's ~60,000. ItemsAdder writes one bitmap provider per font image: 616 in
minecraft:defaultand the same again inminecraft:uniform. That blocks the render thread for over 2 s. The newFontGridMergermerges one-glyph providers that share an image size and every other setting into grid sheets. It leaves a glyph alone when:It decodes ItemsAdder's protected PNGs (scrambled chunk CRCs and zlib checksum) the way the client does. On Main this takes the fonts from 616 providers to 309. The other 247 are the
mcemojisvanilla item emojis, which use vanilla textures the server doesn't have.Zip scanning. In 1.21.10 the client reads every entry of a pack zip once per namespace in that zip, each time it lists resources, and
ZipFileis synchronised. Core held 33 namespaces and 19,795 entries. Core is now split intocore,core2andcore3, using an exact partition of the namespaces sorted by size that minimises namespaces × entries. That cuts the listing work about 5×.The merged fonts and sheets are written as ordinary zip records; every other record is still copied byte for byte. The HTTP server's allowed paths now come from
PackPartitioner.PARTS.Results
Measured on Main's real
generated.zip(cf2c8627), using a vanilla 1.21.10 client with warm F3+T reloads on a 16-thread PC:models.zipandsounds.zipkeep their current hashes, so players' cached copies stay valid.clean verifypasses with full line coverage.Docs: TF-Minecraft/Docs#263
🤖 Generated with Claude Code