Repository navigation
Conversation
…class Wrapping a Java object returned to JS called Class.getName() in Java, copied the jstring, and looked the class up again with FindClass for Link. ObjectManager now keeps up to four runtime classes per declared type, matched with IsSameObject (no Java code runs), and reuses the class name and global class. On a miss FindClass still runs after the wrapper is created, as before.
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Linked class contract and helper test-app/runtime/src/main/cpp/ObjectManager.h, test-app/runtime/src/main/cpp/ObjectManager.cpp |
CreateJSWrapperHelper accepts a class name, an optional class reference, and an optional output pointer for the linked class. It calls FindClass only when no class reference is supplied. |
Instance wrapper cache flow test-app/runtime/src/main/cpp/ObjectManager.h, test-app/runtime/src/main/cpp/ObjectManager.cpp |
ObjectManager stores recent class entries by declared type. It reuses an entry when its class is JNI-identical to the instance’s class and removes the oldest entry when the cache already has four entries. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Suggested reviewers: edusperoni
Merge Risk: 
Minimal · up to f9c8a
-
No material merge-blocking issue was established. The runtime test suite has not been run.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed-
Full details: Title check
-
Explanation
The title names the main change: wrapping a returned object of an already-seen class skips the Class.getName() call. The raw summary and PR objectives both describe a per-class cache in ObjectManager::CreateJSWrapper that reuses the cached class name and class reference on a hit. The title's 'perf:' prefix matches the performance intent stated in the objectives. The title is specific and on topic.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the class cache bright,
And finds a match by JNI sight.
Four linked names are kept in store,
The oldest hops out at the door.
New wrappers spring to life with care.
What
When a Java method returns an object that has no JS wrapper yet,
ObjectManager::CreateJSWrappercalledClass.getName()in Java, copied the jstring into astd::string, replaced.with/, and then looked the class up again withFindClassforLink.ObjectManagernow remembers up to four runtime classes per declared type. Each is matched withIsSameObject, which runs no Java code, and the cached class name and global class are reused. On a miss the old path runs, withFindClassstill after the wrapper is created. The cachedjclassvalues come fromJEnv's class cache, whose global refs are never deleted.Measurements
Pixel 9 Pro emulator (arm64, headless), xplat-benchmarks NativeScript app, size M, release build, 5 interleaved rounds. Both runtimes were built from
mainwith and without this change and #2076 (measured together).attachedB/A:The gain is small; most of the per-object cost is still
getOrCreateJavaObjectID, which this does not touch.Testing
:runtime:assembleRelease -Poptimized -Pabis=arm64-v8abuilds; the runtime swapped into a 9.1.1 app ran every benchmark round.Summary by CodeRabbit