Repository navigation
[native] Remove unused CoreCLR startup arguments - #13355
simonrozsival wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The Java/JNI CoreCLR startup ABI change still requires product builds, comparison with the javac-generated header, and on-device startup validation to establish runtime compatibility.
No open issues 🎉
What changed in this PR
Simplifies the .NET for Android CoreCLR startup contract by removing three arguments that the native host does not use.
Changes:
- Reduces the Java startup arguments from nine to six across the Java and native declarations, caller, and JNI forwarder.
- Removes the redundant runtime-directory lookup and UTC-offset computation.
- Updates handwritten JNI signature comments while preserving app-directory ordering, native-library loading, and assembly discovery.
Product builds, generated-header comparison, and on-device validation remain pending, as noted in the PR description.
| File | Description |
|---|---|
src/native/clr/include/host/host.hh |
Removes unused parameters from the host declaration. |
src/native/clr/include/host/host-jni.hh |
Updates JNI declarations and signature comments. |
src/native/clr/host/host.cc |
Removes unused host parameters and their obsolete comments. |
src/native/clr/host/host-jni.cc |
Updates the JNI entry point and forwarded arguments. |
src/java-runtime/java/mono/android/Runtime.java |
Reduces the native method declaration to six arguments. |
src/java-runtime/java/mono/android/clr/MonoPackageManager.java |
Removes unused startup computations, imports, and arguments. |
Review effort: Balanced
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
CoreCLR startup accepts a runtime native-library directory, a local UTC offset, and an assembly list, but the native host never reads these values. Java still computes the offset using OffsetDateTime or Calendar before passing it across JNI. Remove the three arguments from the Java declaration and caller, JNI forwarder and declarations, and Host entry point. Remove the redundant runtime-directory lookup and timezone/date work used only to populate the unused arguments. Preserve app directory ordering, assembly discovery, and the getAssemblies() API. Validation is limited to source inspection and git diff --check. Product builds, generated JNI header checks, on-device startup tests, and size/performance measurements remain pending the exclusive shared-host validation lease; no measured size or performance improvement is claimed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the localDateTimeOffset parameter and existing Java offset computation available for the DateTime runtime performance work. Restore the parameter throughout the Java/JNI/native startup contract while retaining removal of the unused native-library directory and assembly list arguments. Source inspection confirms the offset computation matches the original baseline and the Java/JNI/native argument ordering and descriptor comments agree. Product builds and device validation remain pending the exclusive validation lease. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep localDateTimeOffset as a plain startup parameter for the DateTime runtime performance work. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
c7247d6 to
0f6a286
Compare
There was a problem hiding this comment.
Review — ⚠️ Needs Changes (validation pending)
Findings: 0 errors, 0 warnings, 1 non-blocking suggestion. No correctness regression found in the six-file change. The inline suggestion consolidates the duplicated JNI declaration; it is not a merge blocker.
Reviewed the complete changed files at head 0f6a28665967b8c3bacbe7fb0cdea07c291da5d2 via GitHub after discovering that the local checkout was the pre-rebase c7247d67 commit. The Java declaration/caller, JNI forwarder, and native declarations agree on the seven-argument ordering. Both removed arguments were unused by the host. The rebased host retains the actual localDateTimeOffset consumption added by #13357. App-directory ordering, library loading, assembly discovery, and getAssemblies() remain intact; the existing Java build regenerates the JNI header from Runtime.java.
CI is not yet green: Azure build 1657585 has five GitHub checks in progress and the aggregate check queued; CLA passed, and no failed checks are currently reported. Direct Azure build/timeline queries were unavailable in this environment, so the Azure result and full job inventory could not be confirmed. Local javac -h validation was denied by execution permissions; no builds or device tests were run by this review. The PR's reported executable validation predates the rebase and does not establish a passing current-head build. The verdict reflects outstanding validation, not a confirmed code defect.
Generated by Android PR Reviewer for #13355 · copilot · gpt61 · 32.5 AIC · ⌖ 7.88 AIC · ⊞ 19.3K
Comment /review to run again
Remove the duplicate JNI initInternal declaration and signature comment so subsequent startup contract updates have one handwritten declaration to maintain. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CoreCLR startup accepts a runtime native-library directory and an assembly list, but
Host::Java_mono_android_Runtime_initInternal()never reads either value. Passing them through Java and JNI leaves unnecessary arguments and obsolete comments in the startup contract.Remove these two arguments from the Java declaration and caller, JNI forwarder and declarations, and native host entry point. Remove the redundant runtime-directory lookup, synchronize the handwritten JNI signature comment, and consolidate the duplicate JNI declaration into one. The existing
javac -hbuild step generates its header from the updated Java declaration.Retain
localDateTimeOffsetthroughout the Java/JNI/native contract, including the existingOffsetDateTime/Calendarcomputation and the native offset consumption introduced onmainby #13357. The rebase conflict resolution preserves the upstream startup function body verbatim.This reduces the startup contract from nine Java arguments to seven and removes 28 net source lines across six files relative to the updated base. App directory ordering, native-library loading, assembly discovery, and
MonoPackageManager.getAssemblies()are preserved. No dependencies or linkage configuration change.Current source validation:
git diff --checkpassed. Source assertions after the rebase confirmed that the upstream startup body (including offset consumption) is unchanged and the Java/JNI/native argument ordering agrees. After consolidating the duplicate declaration,clang++ -fsyntax-onlywith the generated and handwritten headers included together passed. Full builds and device tests have not been rerun after the rebase; shared build/device operations require the coordinator's exclusive lease.Previous executable validation, on pre-rebase commit c7247d6:
make prepareandDOTNET_CLI_USE_MSBUILD_SERVER=0 make prepare CONFIGURATION=Release— passed.DOTNET_CLI_USE_MSBUILD_SERVER=0 make leeroy CONFIGURATION=Release MSBUILD_ARGS='-nr:false'— passed, including extra API levels and local workload setup. CoreCLR native ARM, ARM64 and x64 debug/release builds compile and link. The then-unused offset parameter produced a warning; builds succeeded.javac -houtput agreed with the handwritten JNI declarations. Aclang++ -fsyntax-onlyprobe including both generated and handwritten headers passed.javap -s -cand packaged DEX inspection confirmed the seven-argument contract and retained offset computation.emulator-5554— 991 passed, seven skipped, zero failed (998 total). UsedRuntimeIdentifier=android-arm64,UseMonoRuntime=false, and an explicit per-Microsoft.NETCore.Appframework-reference pin to runtime12.0.0-alpha.1.26523.103under SDK12.0.100-alpha.1.26523.103.The previous device commands used the following shape, with
$ARTIFACTSpointing to the saved validation directory andq-pin-test-runtime.targetscontaining an evaluation-time<FrameworkReference Update="Microsoft.NETCore.App" RuntimeFrameworkVersion="12.0.0-alpha.1.26523.103" />:For that pre-rebase run, verified that fresh native output, local runtime pack, built APK, and pulled installed APK contained identical
libmonodroid.sobytes (SHA-2568cbee48c1f52b00f1b8bf0e1436aedbff1e6eae609ae4479ba732c7b11984f98). Installed Java DEX and CoreCLR bytes also matched the built APK; packaged CoreCLR/JIT/compression libraries matched the explicitly pinned runtime package. ARM/ARM64/x64 native dependency inspection showed no libc++ dependency. These hashes describe the previous build, not the rebased source.ARM32/x64 execution and the pre-API26 offset branch were not exercised. No matched ELF/Java/APK size comparison or performance measurements were performed; no measured app-size savings or performance/no-regression claim is made.