Repository navigation
perf: add app.pack and record javac.pack from a realistic compile - #76
Conversation
…cord javac.pack from a realistic compile
|
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 15 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
![project516-review-bot[bot]](https://avatars.githubusercontent.com/in/4903871?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a new jdk/app.pack for typical program byte ranges beyond a bare start, and re-records jdk/javac.pack to contain only the compiler's own modules. The build script now records from realistic workloads (App.java with lambdas, streams, time, I/O, threads, and a javax.tools compile with annotation processor). The boot pack index is updated to support multiple lazy packs per file, and the browser HTTP layer handles the new structure. Performance tests show reduced cold-start bytes for compile workloads.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict approve
| const beyondBoot = (rec: Recorder) => new Map([...rec.ranges].map(([p, r]) => [p, subtract(merge(r), bootRanges.get(p) ?? [])])); | ||
| const appRanges = beyondBoot(appRecorder), javacRanges = new Map<string, [number, number][]>(); | ||
| for (const [p, r] of beyondBoot(javacRecorder)) { | ||
| const own = p === "/lib/modules" ? subtract(r, subtract(r, compiler)) : []; |
There was a problem hiding this comment.
This computes the intersection of r and compiler via double subtraction. It works but is hard to read. Consider a helper intersect(a, b) or at least a comment explaining the intent: own is the part of r that belongs to compiler modules.
There was a problem hiding this comment.
Agreed that it reads poorly. own is the part of r inside the compiler modules. Each push here reruns the full OpenJDK suite (about 25 minutes), so the intersect(a, b) helper and its comment go in with the next change to this script rather than as a push of their own.
| ...javacFiles.map((p): [string, Uint8Array] => [p, javacRecorder.read(p)!]), | ||
| ...rangeEntries(javacRanges, javacRecorder), | ||
| const appPack = writePack(join(dist, "jdk", "app.pack"), [ | ||
| ...appFiles.map((p): [string, Uint8Array] => [p, jdkFiles.read(p)!]), |
There was a problem hiding this comment.
Non-null assertion on jdkFiles.read(p). If a file recorded during appRecorder/javacRecorder runs is missing from the JDK image (e.g., a race or build inconsistency), the build will crash with a cryptic error. Add a null check and throw a descriptive error.
There was a problem hiding this comment.
This read cannot return null. appFiles holds only paths in the recorders' seen sets, and Recorder.read adds a path to seen only when the read returned data (if (d) this.seen.add(p);, line 102). jdkFiles reads the same build/jdk through the same NodeBackend, in the same build step.
| // program reads one of its parts, holds what the typical program reads beyond | ||
| // that and every java.base part javac reads. javac.pack holds the rest of what | ||
| // javac reads: the compiler's own modules. | ||
| const linked = new Set(/^MODULES="(.*)"$/m.exec(readFileSync(join(jdk, "release"), "utf8"))![1].split(" ")); |
There was a problem hiding this comment.
Parsing the release file with a regex is fragile if the file format changes. The JDK is pinned so this is low risk, but a comment noting the expected format would help future maintenance.
There was a problem hiding this comment.
This is the MODULES="..." line that jlink writes into release. scripts/build-jdk.sh reads it with the same pattern (sed -n 's/^MODULES="\(.*\)"/\1/p'), and src/jvm.ts parses it at every start. If the format ever changed, the ! here would throw at build time, before anything is packed.
| // that and every java.base part javac reads. javac.pack holds the rest of what | ||
| // javac reads: the compiler's own modules. | ||
| const linked = new Set(/^MODULES="(.*)"$/m.exec(readFileSync(join(jdk, "release"), "utf8"))![1].split(" ")); | ||
| const compiler = merge(moduleRuns(readFileSync(join(jdk, "lib", "modules"))) |
There was a problem hiding this comment.
Reading and parsing /lib/modules (a jimage file) to extract compiler module ranges depends on the internal jimage layout. This is build-time only and the JDK is pinned, so acceptable, but worth a comment that it relies on the current jimage structure.
There was a problem hiding this comment.
Agreed, it relies on the jimage layout. That dependence and its check came in #75: moduleRuns reads the jimage index format src/jimage.ts already depends on at run time, and build-web.ts throws (lib/modules has <module> in more than one place) if jlink ever stops laying a module out in one run. So a layout change fails the build instead of producing wrong packs.
What this changes
A cold session fetched most of its JDK bytes as scattered 64 KB Range requests.
javac.packwas recorded from a one-line compile, so a real compile read 7 to 12 MB more from the image files, in 100 to 180 requests. A program that never compiles still fetched the whole ofjavac.packonce it touched one of thejava.baseclasses recorded there.jdk/app.pack, fetched the first time a program reads one of its parts. It holds what a typical program reads beyond a bare start, recorded fromscripts/packs/App.javarun from jars: lambdas and streams,java.time,BigDecimal, threads, file I/O, resources from a jar andSystem.Logger. It also holds everyjava.basepart javac reads.jdk/javac.packnow holds only the compiler's own modules. It is recorded from ajavax.toolscompile with a jar classpath and an annotation processor (scripts/packs/), as well as the browser self test's compile.boot.packis unchanged. Its index can now list more than one lazy pack for a file.Tests
Unit tests for the pack index, including two lazy packs for one file.
Headless Chromium against a static server that compresses like GitHub Pages, cold sessions (empty cache), JDK bytes only. Two rounds, medians; master is the build after feat: put each JDK module but java.base in its own image file #75:
javac.packAt a tenth of one CPU, the TimedRobot's cold run took 39 s instead of 50 s, and the plain template compile took the same time.
Warm sessions (cached) fetch nothing on either.
Noticed, not fixed
app.pack(0.7 MB), and in an earlier run,javac.pack. Something read at a bare start depends on timing, and the boot recording does not always catch it.