Measure virtual thread mounted time - #1
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThe change introduces a Java 25 library for measuring virtual-thread mounted time and mount/unmount counts through a JVMTI native implementation. It adds native loading, integrity checks, Maven packaging, JPMS metadata, runtime and class-loader tests, README documentation, and repository tooling. New CI, reusable native-build, snapshot-release, and release workflows build platform artifacts, validate provenance and packaged libraries, assemble universal packages, and publish releases. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (14)
src/main/cpp/build-native.sh (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
-mmacosx-version-min=15.0narrows the supported macOS range more than the JDK does.Java 25 runs on macOS 13/14, but a library built with a 15.0 minimum emits a
LC_BUILD_VERSIONminimum that older systems reject atdlopentime, soVirtualThreadTime.isSupported()would report unsupported on otherwise valid hosts. Unless a 15-only SDK feature is required, lowering this (e.g.13.0) widens coverage at no cost.src/main/java/io/airlift/vthreadtime/NativeLibrary.java (1)
246-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
normalizeOperatingSystem/normalizeArchitectureNPE if the system property is absent.
os.name/os.archare effectively always set by the JVM, so this is defensive only — but the failure mode (NullPointerExceptionescaping theNativeAccessstatic initializer) is indistinguishable from a genuine bug, whereasUnsupportedOperationExceptionis the documented contract. Consider requiring non-null with a clear message.src/main/java/io/airlift/vthreadtime/NativeAccess.java (1)
178-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStatus codes are duplicated as bare integers against the C
Statusenum.
statusMessagehard-codes 0–13 and the1_000JVMTI offset, mirroring theStatusenum and the+ 1'000convention insrc/main/cpp/vthreadtime.cpp(Lines 43-59, 439) with nothing tying them together. A named constant set (or an enum with explicit codes) plus a comment pointing at the C definition keeps the two ends from drifting silently when a status is added.src/main/java/io/airlift/vthreadtime/VirtualThreadTime.java (1)
99-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse identity comparison for the owner check.
Threaddoes not overrideequals, soowner.equals(...)is already identity — writing it asowner != Thread.currentThread()states the confinement intent directly and avoids implying value semantics.pom.xml (2)
66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclaring
<resources>replaces the defaultsrc/main/resourcesentry.Maven's default resource root is dropped once an explicit
<resources>list is present, so any current or futuresrc/main/resourcescontent silently stops being packaged. Add it back alongside the generated directory.♻️ Proposed fix
<resources> + <resource> + <directory>src/main/resources</directory> + </resource> <resource> <directory>${vthreadtime.native.resources}</directory> </resource> </resources>
71-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe native build is unconditional, so every build requires a C++ toolchain and a supported OS.
build-native.shexits 2 on anything other than Linux/macOS and on a missingJAVA_HOME, and the exec execution is bound togenerate-resourceswith no skip property or platform activation. That makesmvn installunbuildable on unsupported hosts and, per the stack's release flow that supplies pre-built native resources, forces a recompile even when binaries are already provided. A<skip>${vthreadtime.native.skip}</skip>toggle (or an OS-activated profile) keeps both paths workable..github/workflows/native-build.yml (1)
76-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeployment-target assertion depends on
LC_BUILD_VERSIONbeing emitted.If the toolchain emits
LC_VERSION_MIN_MACOSXinstead (older linker paths),minimum_macosis empty and the job fails with a confusing "found " message. Consider also matchingLC_VERSION_MIN_MACOSX/version, or at least emitting the rawotooloutput on mismatch to make triage easy..github/workflows/ci.yml (1)
171-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmit a clear failure message and consider sharing the expected-path list.
grep -Fxfailing relies onset -eand prints nothing useful. The same four expected resource paths are also hardcoded in.github/workflows/snapshot-release.yml(lines 113-123), so they can drift independently.♻️ Proposed change
for library in \ io/airlift/vthreadtime/native/linux-aarch64/libvthreadtime.so \ io/airlift/vthreadtime/native/linux-x86_64/libvthreadtime.so \ io/airlift/vthreadtime/native/macos-aarch64/libvthreadtime.dylib \ io/airlift/vthreadtime/native/macos-x86_64/libvthreadtime.dylib; do - grep -Fx "$library" "${RUNNER_TEMP}/jar-contents.txt" + if ! grep -qFx "$library" "${RUNNER_TEMP}/jar-contents.txt"; then + echo "Missing native library in $main_jar: $library" >&2 + exit 1 + fi done.github/workflows/snapshot-release.yml (1)
111-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a
*)default to thecasefor future platform additions.
expected_pathcarries over from the previous iteration if a new platform is added toplatformswithout a matching branch, which would silently validate against the wrong path.♻️ Proposed change
macos-aarch64) expected_path=io/airlift/vthreadtime/native/macos-aarch64/libvthreadtime.dylib ;; + *) + echo "Unknown platform: $platform" >&2 + exit 1 + ;; esacsrc/test/java/io/airlift/vthreadtime/TestVirtualThreadTime.java (1)
114-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the exception is present explicitly.
If
mountedTimeNanos()unexpectedly succeeds,failureisnulland the failure message becomes a generic "actual not to be null" rather than something describing the missing confinement check.♻️ Proposed tweak
assertThat(failure) + .describedAs("expected thread-confinement failure") + .isNotNull() .hasMessage("Tracker can only be used by its registered virtual thread");src/test/java/io/airlift/vthreadtime/TestNativeLibraryClassLoader.java (4)
324-373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree near-identical loader helpers.
initializeAndRelease,verifyUnsupportedAndRelease, andverifyExpectedFailureAndReleasediffer only in what they assert against the freshly loadedVirtualThreadTime. Extract a singlewithDisposableLoader(URL, ThrowingConsumer<Class<?>>)that owns the loader creation, close, andWeakReferencereturn.
390-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why the
Methodis nulled out.
register.getAndSet(null)exists so the reflectiveMethod(and through it the disposable loader) becomes unreachable beforeawaitCollectedruns. That intent is invisible to a future reader who might "simplify" this to a plain captured local and silently break the GC assertion at Line 285.
435-447: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPolling loop creates and GC-awaits a class loader per iteration.
Each
readActiveRegistrationscall spins up a freshURLClassLoader, resolves the native library, then runsawaitCollected(aSystem.gc()loop). At 10 ms intervals for up to 10 s that is up to ~1,000 loader creations and forced GCs, which is slow and makes the 10 s deadline mostly GC overhead rather than actual wait time.Hold one loader for the duration of the poll and await its collection once after the loop.
197-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
NativeLibrary.Platformfor the packaged native-library path.
packagedNativeLibrary()duplicates the platform mapping, OS normalization, architecture aliases, and library-name generation already central inNativeLibrary. Make the accessor/package path available to this same-package test and derive the resource path from the main-platform logic so the packaged resource lookup cannot drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a682693-a72a-4d0c-aad9-067abcff88e2
📒 Files selected for processing (15)
.github/workflows/ci.yml.github/workflows/native-build.yml.github/workflows/release.yml.github/workflows/snapshot-release.ymlREADME.mdpom.xmlsrc/main/cpp/build-native.shsrc/main/cpp/vthreadtime.cppsrc/main/java/io/airlift/vthreadtime/NativeAccess.javasrc/main/java/io/airlift/vthreadtime/NativeLibrary.javasrc/main/java/io/airlift/vthreadtime/VirtualThreadTime.javasrc/main/java/io/airlift/vthreadtime/package-info.javasrc/main/java/module-info.javasrc/test/java/io/airlift/vthreadtime/TestNativeLibraryClassLoader.javasrc/test/java/io/airlift/vthreadtime/TestVirtualThreadTime.java
af41fda to
5ba32aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
.github/workflows/release.yml (2)
6-16: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winWorkflow-level
permissions:block is still missing.A prior review flagged this and it was marked addressed, but the current file has no top-level
permissions:block, and thenative-buildjob (15-16) has no explicit permissions either - it will run with the default token permissions rather than a least-privilege grant. (Note:native-build.ymlitself declarespermissions: contents: read, which acts as an effective ceiling, so actual exposure is likely limited - but making it explicit here removes any ambiguity.)🔒 Proposed fix
concurrency: group: maven-publication cancel-in-progress: false +permissions: + contents: read + defaults:Source: Linters/SAST tools
15-19: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRelease path still doesn't pin the native-build commit or verify provenance/digests before publishing.
A prior review raised this and it was marked addressed, but the current workflow still calls
native-build(15-16) without arefinput, and thereleasejob never verifies the downloaded native artifacts' provenance or packaged digests beforenjord:publish(101-106) - unlikesnapshot-release.yml, which resolves a specific commit (23-38), pins it asref(39-44), and verifies commit/platform/digest for every native library (85-151) plus the packaged jar's digest (171-197) before publishing.Even if
github.shahappens to stay consistent between the two checkouts by default GitHub Actions behavior here, this workflow publishes to Maven Central with no integrity check on the native binaries it bundles - the same defense-in-depth gap the earlier review called out. Recommend mirroring theresolve-source+ provenance/digest-verification pattern fromsnapshot-release.yml.Also applies to: 91-99
🧹 Nitpick comments (2)
src/main/c/vthreadtime.c (1)
52-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ThreadStatefields are plain scalars but touched from different carrier threads.A virtual thread's state is written on the carrier that unmounts it and later read/updated on a different carrier after remount. The JVM's mount/unmount transitions almost certainly provide the needed happens-before, but strictly this is an unsynchronized cross-thread access; consider relaxed atomics (or a comment recording the assumed ordering guarantee) to make the intent explicit.
Also applies to: 139-168
src/main/java/io/airlift/vthreadtime/NativeLibrary.java (1)
219-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading
ignoredname for a stream that is used.The
DigestInputStreamnamedignoredis the stream being read; rename for clarity.♻️ Proposed rename
- try (InputStream input = Files.newInputStream(path); - DigestInputStream ignored = new DigestInputStream(input, digest)) { - ignored.transferTo(OutputStream.nullOutputStream()); - } + try (InputStream input = Files.newInputStream(path); + DigestInputStream digestInput = new DigestInputStream(input, digest)) { + digestInput.transferTo(OutputStream.nullOutputStream()); + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae7f580a-0101-4cdd-82ba-37b2a68807af
📒 Files selected for processing (26)
.github/dependabot.yml.github/release.yml.github/workflows/ci.yml.github/workflows/native-build.yml.github/workflows/release.yml.github/workflows/snapshot-release.yml.gitignore.java-version.mvn/errorprone.config.mvn/jvm.config.mvn/maven.config.mvn/settings.xml.mvn/wrapper/maven-wrapper.propertiesLICENSEREADME.mdmvnwpom.xmlsrc/main/c/build-native.shsrc/main/c/vthreadtime.csrc/main/java/io/airlift/vthreadtime/NativeAccess.javasrc/main/java/io/airlift/vthreadtime/NativeLibrary.javasrc/main/java/io/airlift/vthreadtime/VirtualThreadTime.javasrc/main/java/io/airlift/vthreadtime/package-info.javasrc/main/java/module-info.javasrc/test/java/io/airlift/vthreadtime/TestNativeLibraryClassLoader.javasrc/test/java/io/airlift/vthreadtime/TestVirtualThreadTime.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/io/airlift/vthreadtime/package-info.java
electrum
left a comment
electrum
left a comment
There was a problem hiding this comment.
Reviewed src/main/c/vthreadtime.c at 2e86627fc1875256ce3ae9982b9c797c595be00a. I left one inline correctness/reliability concern about disposing the JVMTI environment while callbacks can still be in flight.
Other findings:
- The carrier-local
_Thread_local currentStatedesign looks sound for HotSpot: mount populates it, the virtual thread's native reads execute on that carrier, unmount clears it, and a later mount repopulates it on the next carrier. - Non-blocking hardening: check
SetThreadLocalStorage(..., NULL)before freeing the state; distinguish a JVMTI lookup error from an empty TLS slot ingetState(); and validate the extension-event parameter metadata before parsing variadic callback arguments. - Validation: all 15 project tests passed locally; GitHub CI is green; Clang static analysis reported no diagnostics; and a targeted stress test completed 200,000 park/remount transitions while preserving monotonic mounted time and paired mount/unmount counts.
- A late-initialization fault-injection probe repeatedly observed callbacks being delivered during the failure/rollback window, but I did not reproduce a crash on the tested HotSpot 25 build.
| success = false; | ||
| } | ||
| } | ||
| if ((*environment)->DisposeEnvironment(environment) != JVMTI_ERROR_NONE) { |
There was a problem hiding this comment.
Disabling and uninstalling the callbacks does not drain callbacks already in flight. The JVMTI DisposeEnvironment contract explicitly says that currently running handlers continue while the environment becomes invalid. If enabling a later notification fails, another carrier may already be executing onMount or onVirtualThreadEnd, both of which call through environment.
Please either retain the disabled environment after rollback or add an in-flight callback barrier before disposing it. The fault-injection test should also exercise a late initialization failure while virtual threads are actively transitioning; a targeted probe repeatedly observed callbacks being delivered during this window.
https://docs.oracle.com/en/java/javase/25/docs/specs/jvmti.html#dispose-environment
There was a problem hiding this comment.
Agreed. Failed initialization now retains the JVMTI environment for the JVM lifetime, performs best-effort notification and callback cleanup, and preserves the original failure. The fault test exercises late failures while virtual threads are transitioning, verifies callback delivery stops afterward, and now directly probes the retained environment after cleanup. I also verified the negative case: restoring DisposeEnvironment causes the test to fail with JVMTI_ERROR_INVALID_ENVIRONMENT.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
15-16: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd explicit
permissionsto thenative-buildjob.
native-build(Line 15-16) calls the reusable workflow without apermissionsblock. Per GitHub's documented behavior, "If jobs.<job_id>.permissions is not specified in the calling job, the called workflow will have the default permissions for the GITHUB_TOKEN." This still leavesnative-buildrunning with the repository's default token permissions instead of least privilege, which matches the zizmor warning still reported for these lines. A prior review raised a related permissions concern for this file that was marked addressed, but the fix only added apermissionsblock to thereleasejob (Line 22-24), not tonative-build.🔒 Proposed fix
native-build: + permissions: + contents: read uses: ./.github/workflows/native-build.ymlConfirm with
native-build.yml's own required scopes so the grant here matches what it actually needs.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 59d43296-2ccb-4116-a514-18915c76fea8
📒 Files selected for processing (26)
.github/dependabot.yml.github/release.yml.github/workflows/ci.yml.github/workflows/native-build.yml.github/workflows/release.yml.github/workflows/snapshot-release.yml.gitignore.java-version.mvn/errorprone.config.mvn/jvm.config.mvn/maven.config.mvn/settings.xml.mvn/wrapper/maven-wrapper.propertiesLICENSEREADME.mdmvnwpom.xmlsrc/main/c/build-native.shsrc/main/c/vthreadtime.csrc/main/java/io/airlift/vthreadtime/NativeAccess.javasrc/main/java/io/airlift/vthreadtime/NativeLibrary.javasrc/main/java/io/airlift/vthreadtime/VirtualThreadTime.javasrc/main/java/io/airlift/vthreadtime/package-info.javasrc/main/java/module-info.javasrc/test/java/io/airlift/vthreadtime/TestNativeLibraryClassLoader.javasrc/test/java/io/airlift/vthreadtime/TestVirtualThreadTime.java
🚧 Files skipped from review as they are similar to previous changes (20)
- .mvn/jvm.config
- src/main/java/module-info.java
- .gitignore
- .java-version
- .mvn/settings.xml
- .github/dependabot.yml
- LICENSE
- .github/release.yml
- .mvn/wrapper/maven-wrapper.properties
- .mvn/maven.config
- src/main/java/io/airlift/vthreadtime/package-info.java
- pom.xml
- .mvn/errorprone.config
- README.md
- src/test/java/io/airlift/vthreadtime/TestVirtualThreadTime.java
- .github/workflows/snapshot-release.yml
- src/main/java/io/airlift/vthreadtime/NativeAccess.java
- src/main/c/build-native.sh
- .github/workflows/ci.yml
- src/main/c/vthreadtime.c
Establish the dependency-free Java 25 project scaffold, current Airlift build checks, and manual release workflows before native implementation begins.
Thread CPU time is unavailable for virtual threads, but schedulers need to distinguish active work from time spent parked. Use HotSpot JVMTI mount and unmount callbacks to measure carrier-mounted intervals for registered virtual threads and expose that data through a dependency-free Java 25 API.
2 participants
Thread CPU time is unavailable for virtual threads, but schedulers need to distinguish active work from time spent parked. Use HotSpot JVMTI mount and unmount callbacks to measure carrier-mounted intervals for registered virtual threads and expose that data through a dependency-free Java 25 API.