test: add JSICache leak repro harness test (#1464) - #1518
Open
hmelonjp wants to merge 1 commit into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Owner
|
Nice! Clean and efficiently. Let's see CI |
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
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.
What this is
A repro-only PR that illustrates the
JSICacheleak described in #1464.#1467's thread asked for exactly this:
This doesn't attempt a fix - just a deterministic, CI-runnable test that
shows the leak is real, using the existing leak-test pattern already in
example/src/getTests.ts('HybridObjects do not leak memory when automatically reclaimed by JS GC') as the template.What it adds
JSICacheReference::getTotalSize()(JSICache.hpp) — sums the six cachevectors' sizes for the current Runtime. Debug-only introspection, no
behavior change.
HybridNitroModulesProxy::debug_getTotalJSICacheSize()— exposes that toJS, wired the same way as the existing
debug_getTotalAllocatedHybridObjects().NitroModules.debug_getTotalJSICacheSize()TS declaration.'JSICache does not leak memory (nitro#1464)', next tothe existing HybridObject leak test: calls
bounceArrayBuffers([new ArrayBuffer(8)])55,000 times with fresh plain ArrayBuffers (noNativeState, so each one takes themakeShared(...)path), GCs, andasserts the JSICache slot count grew by less than 10% of the iteration
count — same threshold the existing HybridObject test uses.
Verified locally
On this PR, against unpatched
main, the test fails on both platforms -confirming it actually reproduces the leak:
Note, not a claim about any fix: out of curiosity I tried this test against
a local build with #1467/#1468/#1469 applied. iOS came back fully clean,
both variants. Android's C++ variant also came back clean. Android's
Swift/Kotlin variant was better than unpatched but still failed across
several runs - somewhere between 0% and 25% of the growth reclaimed
depending on the run, never close to the 90% the test needs, and flat
across several more seconds of GC + waiting when checked (not just slow to
catch up). Didn't dig into why - out of scope for this PR, just flagging it
in case it's useful.
Steps used to reproduce the failure, in case anyone wants to run this
locally:
A few non-obvious things that cost real time getting a clean run on both
platforms, noting them here so they're not rediscovered:
HARNESS_APP_PATHis required (both platforms) and not documentedanywhere in this repo - without it,
getHarnessAppPath()throwssynchronously before Metro/the platform runner ever start, and on iOS that
error gets swallowed by an unrelated
AbortErrorracing it out of theother half of the
Promise.allincreateHarnessSession- the actualcause never reaches stdout. If that's worth a separate issue, happy to
file it.
via
simctl appinfo/ equivalent on Android). It does not checkwhether the installed binary is stale, so re-running after a rebuild
silently tests the old binary unless you uninstall first. Bit us more than
once - a "clean" run reporting the exact same numbers as a previous one is
a sign this happened.
console.loginto its own stdout.adb logcat(tagReactNativeJS) works; no equivalent found for iOS inthe time we spent on this.
Scope
This PR only illustrates the issue - it's a repro, not a fix.
Checklist
main, narrow scope (one test + the debug accessor it needs).fails in CI on this PR alone (see numbers above).
bun specsrun - no diff. This PR doesn't touch any.nitro.tsspec,so nitrogen output is unaffected.
clang-format -style=file:./config/.clang-formaton the threetouched C++ files and
eslinton the two touched TS files, both clean andidempotent. (Did not run
bun lint-allwholesale - my local clang-formatis v22 against the repo's pinned v16, and running it repo-wide risked
reformatting unrelated files under a different binary than CI uses. Happy
to have CI's own lint job confirm instead.)
test: add JSICache leak repro harness test (#1464).