You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf: guard ObjectTable with a monitor instead of StampedLock - #2103
ObjectTable (added in #2099) guarded its tables with java.util.concurrent.locks.StampedLock: write locks for mutations, optimistic reads elsewhere. On device, StampedLock runs interpreted. It lives in the boot classpath (core-oj) but outside the boot image, and an app's baseline profile can't compile boot-classpath methods. A profile of NativeScript core's apps/automated suite showed StampedLock.tryWriteLock/casState/writeLock/unlockWriteInternal running in the interpreter on the main thread.
This replaces it with synchronized (this). An uncontended monitor enter/exit is compiled into every caller. Reads now take the lock too, so the torn-read handling the optimistic path needed (findIdUnlocked, getUnlocked) is gone. A second commit makes the table concurrent on every API level. It was limited to API 24+ only because StampedLock needs it. On API 21–23, argument objects were resolved natively under the isolate lock and new ids took a separate path; now every level registers through findOrRegister. findIdOptimistic is merged into findId, and getOrCreateObjectId takes one lock instead of a lookup followed by a registration.
JNI microbenchmarks (debug.ns.jnibench, optimized release, SM-G780G; main and this branch interleaved, 2 passes × 3 runs; median ns):
Benchmark
main
this PR
new java.lang.Object()
2763
1528
−45% (6/6 runs faster)
lifecycle new java.lang.Object() [alloc, kept alive]
contention, 2 threads, 3 new objects of 16 classes
6295
6487
+3% (0/6)
contention, 4 threads, 3 existing objects
3343
3546
+6% (0/6)
The other rows are within ±2%. Even after the JIT has compiled it, StampedLock's write path was costly.
Known trade-off: when 2–4 Java threads call into JS at the same time, their object-id lookups used to be lock-free optimistic reads and now contend on the monitor: +3..+6% in those benchmarks. Apps run JS on one thread in the common case, where this is a clear win.
Real apps: no measurable difference on the template app's cold start (497/491 vs 503/494 ms) or on a 1000-tile flex-wrap mount. The phone was thermally throttling during that comparison, so those numbers only rule out a large effect.
Does your commit message include the wording below to reference a specific issue in this repo?
Yes. The ObjectTable JVM test for concurrent readers and a writer now requires every read of a stable entry to hit; it used to allow optimistic misses. The test for disabled optimistic reads is gone with the flag. JVM tests: 29/29.
Full device suite:
API 36 emulator: 1551 specs, 0 failures.
API 23 emulator (arm64): 1551 specs, 3 failures, the same 3 that main fails on that emulator:
a proxy test that matches ByteBuffer.toString()'s newer cap= format;
two HTTP module-import tests that time out.
On API 23 the harness also reports "App has crashed - ErrorReportActivity" for both main and this PR. run-as ... ls exits 0 there even when the file is missing, so the harness's checks misfire; the results were read from the XML directly.
Summary by CodeRabbit
Bug Fixes
Improved the reliability of object lookups when multiple threads access the runtime, helping ensure stable objects return the correct identifiers.
Object registration, lookup, retrieval, and removal now use consistent synchronization, reducing the risk of inconsistent results during concurrent access.
Updated object argument handling to register objects without an existing identifier and correctly mark newly registered objects.
StampedLock lives in the boot classpath but outside the boot image, so on
device it runs interpreted, and an app's baseline profile can't compile it.
A monitor's uncontended enter/exit is compiled into every caller. Reads now
take the lock too, which also removes the torn-read handling the optimistic
path needed.
No actionable comments were generated in the recent review.
Walkthrough
ObjectTable removes StampedLock and optimistic reads. It synchronizes table reads, registration, and mutations on the table monitor. Runtime uses the synchronized lookup and registration methods, and concurrency tests require stable lookup results.
ObjectTable removes StampedLock and optimistic lookup paths. It synchronizes lookup, registration, retrieval, containment, weakening, and removal on the table monitor. Batch weakening holds the monitor across the batch.
Runtime lookup and registration test-app/runtime/src/main/java/com/tns/Runtime.java
Runtime uses findOrRegister for object ID lookup and registration, and uses findId for native dispatch. Argument packaging marks newly registered objects with FRESH_ARG.
The concurrent-read test uses findId and requires the expected ID. Tests no longer configure optimistic reads or assert that optimistic reads return NO_ID.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
ObjectTable was only concurrent where StampedLock exists (API 24+). With a
monitor there is no reason to keep API 21-23 on the slower paths: argument
objects resolved natively under the isolate lock, and new ids registered
through a separate path. findIdOptimistic is now findId, and
getOrCreateObjectId and packageArgs register through findOrRegister alone.
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
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.
Description
ObjectTable(added in #2099) guarded its tables withjava.util.concurrent.locks.StampedLock: write locks for mutations, optimistic reads elsewhere. On device,StampedLockruns interpreted. It lives in the boot classpath (core-oj) but outside the boot image, and an app's baseline profile can't compile boot-classpath methods. A profile of NativeScript core'sapps/automatedsuite showedStampedLock.tryWriteLock/casState/writeLock/unlockWriteInternalrunning in the interpreter on the main thread.This replaces it with
synchronized (this). An uncontended monitor enter/exit is compiled into every caller. Reads now take the lock too, so the torn-read handling the optimistic path needed (findIdUnlocked,getUnlocked) is gone. A second commit makes the table concurrent on every API level. It was limited to API 24+ only becauseStampedLockneeds it. On API 21–23, argument objects were resolved natively under the isolate lock and new ids took a separate path; now every level registers throughfindOrRegister.findIdOptimisticis merged intofindId, andgetOrCreateObjectIdtakes one lock instead of a lookup followed by a registration.JNI microbenchmarks (
debug.ns.jnibench, optimized release, SM-G780G;mainand this branch interleaved, 2 passes × 3 runs; median ns):new java.lang.Object()new java.lang.Object()[alloc, kept alive]JniBench.newObjectInJava()[alloc, kept alive]static.newObjectOfManyClasses()[16 classes]static.newObjectInJava()[new wrapper]static.newInstance(extended class)[Java-created]new Object.extend({})()callback.noArgs()The other rows are within ±2%. Even after the JIT has compiled it,
StampedLock's write path was costly.Known trade-off: when 2–4 Java threads call into JS at the same time, their object-id lookups used to be lock-free optimistic reads and now contend on the monitor: +3..+6% in those benchmarks. Apps run JS on one thread in the common case, where this is a clear win.
Real apps: no measurable difference on the template app's cold start (497/491 vs 503/494 ms) or on a 1000-tile flex-wrap mount. The phone was thermally throttling during that comparison, so those numbers only rule out a large effect.
Does your commit message include the wording below to reference a specific issue in this repo?
No issue.
Related Pull Requests
ObjectTable.Does your pull request have unit tests?
Yes. The
ObjectTableJVM test for concurrent readers and a writer now requires every read of a stable entry to hit; it used to allow optimistic misses. The test for disabled optimistic reads is gone with the flag. JVM tests: 29/29.Full device suite:
mainfails on that emulator:ByteBuffer.toString()'s newercap=format;On API 23 the harness also reports "App has crashed - ErrorReportActivity" for both
mainand this PR.run-as ... lsexits 0 there even when the file is missing, so the harness's checks misfire; the results were read from the XML directly.Summary by CodeRabbit