| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 82d98f3d-f95c-4110-b910-00addd480d6d 📥 CommitsReviewing files that changed from the base of the PR and between 9b20dd2 and edbe32a. 📒 Files selected for processing (2)
📝 Walkthrough WalkthroughThe runtime adds a WHATWG Performance API, native timing integration, and a dedicated Android frame-callback bridge. The test app validates performance timestamps and frame implementations. Documentation describes supported APIs, platform behavior, and specification deviations. ChangesPerformance runtime
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JavaScript
participant FrameCallbacks
participant AndroidChoreographer
participant V8
JavaScript->>FrameCallbacks: postFrameCallback()
FrameCallbacks->>AndroidChoreographer: schedule callback
AndroidChoreographer->>FrameCallbacks: deliver frame timestamp
FrameCallbacks->>V8: invoke callback with timing values
Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
…ine)
Replaces the bare native {now, timeOrigin} object with a spec-shaped
implementation of High Resolution Time, User Timing Level 3 and the
Performance Timeline with PerformanceObserver. performance, Performance,
PerformanceEntry, PerformanceMark, PerformanceMeasure, PerformanceObserver
and PerformanceObserverEntryList are globals in the main and worker isolates
alike, with Performance extends EventTarget and WebIDL-shaped descriptors
and brands.
All spec logic lives in the internal/performance.js builtin, which the
native side feeds exactly two values -- binding.now() and
binding.timeOrigin -- so the file is shared verbatim with the iOS runtime.
tns::Performance::NowMillis(isolate) is the single native clock hook a
future requestAnimationFrame must read, so every JS-visible timestamp
shares performance.timeOrigin as its base. Time origins stay per-Runtime,
captured in PrepareV8Runtime, so each worker keeps its own.
mark/measure detail is structured-cloned at entry creation through the
structuredClone global installed by StructuredClone::Init, so entries hold
snapshots and an uncloneable detail throws the DataCloneError-named error;
the builtin keeps an identity fallback for a runtime that ships the
Performance API before structuredClone.
Mirrors NativeScript/ios#430 and 6dd55238d.
__postFrameCallback now hands its callback two arguments,
(frameTimeNanos, performanceMillis). The first is unchanged -- the
platform's raw CLOCK_MONOTONIC frame time, which shipped app code divides
by 1e6 -- and the second is that same instant on the isolate's performance
timeline, so it compares directly with performance.now(). Choreographer
stamps frames on the clock the time origin is captured on, so the
conversion (Performance::MonotonicNanosToTimelineMillis, subtracting the
new Runtime::TimeOriginMonotonicMillis) is exact rather than a resampling.
The machinery moves out of CallbackHandlers into FrameCallbacks.{h,cpp},
which now covers the whole minSdk range: AChoreographer only exists from
API 24, so below it __postFrameCallback silently never fired. API 21-23 now
goes through android.view.Choreographer via com.tns.FrameCallbacks, holding
the same entry and producing the same two arguments. Entries are stored
behind unique_ptr so both implementations can hand the platform a stable
pointer, isolate teardown no longer erases while iterating, and the frame
time from the pre-API-29 AChoreographer entry point is widened to 64 bits,
which it is not on the 32-bit ABIs.
Debug runtimes expose __setFrameCallbackImpl so the Java bridge is
selectable on a modern device; both implementations are covered by specs.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)test-app/runtime/src/main/cpp/Performance.cpp (1)🤖 Prompt for all review comments with AI agentstest-app/runtime/src/main/cpp/js/performance.js (1)25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Read the isolate from the context.
context->GetIsolate() returns the isolate that owns the context. It removes the dependence on the current-isolate thread-local.
♻️ Proposed refactor🤖 Prompt for AI Agentsvoid Performance::Init(Local<Context> context) { - Isolate* isolate = Isolate::GetCurrent(); + Isolate* isolate = context->GetIsolate();Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/Performance.cpp` around lines 25 - 27, Update Performance::Init to obtain the isolate via context->GetIsolate() instead of Isolate::GetCurrent(), ensuring it uses the isolate that owns the provided context.docs/performance.md (1)599-605: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Install the globals as non-enumerable properties.
Plain assignment makes these properties enumerable. WebIDL defines interface objects and the performance attribute as non-enumerable, writable, and configurable. The rest of this file already reproduces the WebIDL shape through finishInterface, so the globals should match.
♻️ Proposed refactor for the global installation🤖 Prompt for AI Agents-g.Performance = Performance; -g.PerformanceEntry = PerformanceEntry; -g.PerformanceMark = PerformanceMark; -g.PerformanceMeasure = PerformanceMeasure; -g.PerformanceObserver = PerformanceObserver; -g.PerformanceObserverEntryList = PerformanceObserverEntryList; -g.performance = new Performance(kInternal); +function defineGlobal(name, value) { + ObjectDefineProperty(g, name, { + value: value, + writable: true, + enumerable: false, + configurable: true, + }); +} +defineGlobal("Performance", Performance); +defineGlobal("PerformanceEntry", PerformanceEntry); +defineGlobal("PerformanceMark", PerformanceMark); +defineGlobal("PerformanceMeasure", PerformanceMeasure); +defineGlobal("PerformanceObserver", PerformanceObserver); +defineGlobal("PerformanceObserverEntryList", PerformanceObserverEntryList); +defineGlobal("performance", new Performance(kInternal));Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-app/runtime/src/main/cpp/js/performance.js` around lines 599 - 605, Update the global installations for Performance, PerformanceEntry, PerformanceMark, PerformanceMeasure, PerformanceObserver, PerformanceObserverEntryList, and performance to use non-enumerable, writable, configurable property definitions instead of plain assignment, matching the WebIDL property shape established by finishInterface.78-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Document the debug-only __setFrameCallbackImpl override.
FrameCallbacks::Init installs __setFrameCallbackImpl under APPLICATION_IN_DEBUG, and testPostFrameCallback.js depends on it to reach the Java bridge. The section describes both implementations but does not mention that the selection can be forced in debug builds. Add a sentence so the next reader does not rediscover it from the source.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/performance.md` around lines 78 - 82, Update the documentation section describing the NDK and Java Choreographer implementations to mention that FrameCallbacks::Init installs the debug-only __setFrameCallbackImpl override under APPLICATION_IN_DEBUG, allowing testPostFrameCallback.js to force the Java bridge path.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/performance.md`:
- Around line 37-39: Update the PerformanceObserver description to state that
supportedEntryTypes contains “mark” and “measure” rather than asserting array
identity with `===`; preserve the surrounding API summary.
In `@test-app/app/src/main/assets/app/tests/testPostFrameCallback.js`:
- Around line 191-197: Update the reference-clock calculation near
originFromClock to sample performance.now() immediately before and after
java.lang.System.nanoTime(), then use the performance timestamp midpoint when
computing the origin. Preserve the existing origin comparison and tolerance
while ensuring the two clock reads are bracketed.
In `@test-app/runtime/src/main/cpp/FrameCallbacks.cpp`:
- Around line 196-201: Protect all accesses to the shared entries_ map with one
std::mutex, including emplace, find, erase, and RemoveIsolateEntries iteration;
release the lock before invoking cb->Call while retaining the needed callback
data safely. Also replace lazy initialization in ResolveChoreographer for
choreographerResolved_ and its dlsym pointers, and in PostJava for
FRAME_CALLBACKS_CLASS, FRAME_CALLBACKS_CTOR, FRAME_CALLBACKS_POST, and
FRAME_CALLBACKS_RELEASE, with std::call_once or function-local statics.
- Around line 238-250: Wrap the Dispatch calls in OnNativeFrame32 and
OnNativeFrame64 with exception handling so NativeScriptException cannot escape
the AChoreographer C callbacks. Catch the exception inside each callback and
report it using the existing exception-reporting mechanism, while preserving the
current timestamp conversion and callback-entry dispatch behavior.
In `@test-app/runtime/src/main/cpp/js/performance.js`:
- Around line 484-537: Update the measure() argument normalization around
isOptionsObject so null startOrMeasureOptions and endMark are treated as
omitted, matching WebIDL dictionary conversion. Ensure null does not reach
convertMarkToTimestamp: performance.measure("a", null) must use startTime 0, and
performance.measure("a", "m", null) must use the two-argument behavior with
endTime now().
In `@test-app/runtime/src/main/java/com/tns/FrameCallbacks.java`:
- Line 15: Declare the released field in FrameCallbacks as volatile so doFrame
observes updates made by release() even when teardown occurs on another thread;
leave the existing release and callback logic unchanged.
---
Nitpick comments:
In `@docs/performance.md`:
- Around line 78-82: Update the documentation section describing the NDK and
Java Choreographer implementations to mention that FrameCallbacks::Init installs
the debug-only __setFrameCallbackImpl override under APPLICATION_IN_DEBUG,
allowing testPostFrameCallback.js to force the Java bridge path.
In `@test-app/runtime/src/main/cpp/js/performance.js`:
- Around line 599-605: Update the global installations for Performance,
PerformanceEntry, PerformanceMark, PerformanceMeasure, PerformanceObserver,
PerformanceObserverEntryList, and performance to use non-enumerable, writable,
configurable property definitions instead of plain assignment, matching the
WebIDL property shape established by finishInterface.
In `@test-app/runtime/src/main/cpp/Performance.cpp`:
- Around line 25-27: Update Performance::Init to obtain the isolate via
context->GetIsolate() instead of Isolate::GetCurrent(), ensuring it uses the
isolate that owns the provided context.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ccd90a72-d7e5-4854-8dd9-4f2f7f2f2087
📥 CommitsReviewing files that changed from the base of the PR and between f284059 and 479dd74.
📒 Files selected for processing (19)
Sorry, something went wrong.
The registry holds entries for every isolate in the process, so its lookups now take a mutex -- never held across the JS call, which a self-rescheduling callback re-enters -- and the two lazy-init blocks (the AChoreographer dlsym, the FrameCallbacks method ids) go through call_once. Entries are identified to the platform by id rather than by address, so a frame arriving after its entry was retired resolves to nothing instead of to freed memory, and teardown detaches entries under the mutex and destroys them after releasing it, since the destructor calls into Java. The registry was shared and unguarded before it moved out of CallbackHandlers; this is not a regression from the move. Dispatch no longer throws. On the NDK path it runs inside a C callback in libandroid, which a C++ exception may not unwind through, so a JS exception the runtime still owns goes to Java the way Timers::FireTimer does. com.tns.FrameCallbacks.released becomes volatile: it is set from runtime teardown, which is not necessarily the frame thread. measure() treats a null startOrMeasureOptions as absent. WebIDL converts null for a (DOMString or PerformanceMeasureOptions) union to an empty dictionary, so it means "no options", not the mark name "null". A null endMark keeps throwing: that parameter is a plain DOMString, neither a union nor nullable, so null stringifies per WebIDL.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-app/app/src/main/assets/app/tests/testPerformance.js`:
- Around line 37-46: Add performance.mark("the-start") before the
performance.measure call in the “Should reject a null end mark” test, ensuring
the existing start mark is present so the assertion specifically validates null
endMark handling.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76065f26-8f35-4c20-9490-589b986d461d
📥 CommitsReviewing files that changed from the base of the PR and between 479dd74 and 9b20dd2.
📒 Files selected for processing (7)
Sorry, something went wrong.
The spec measured from "the-start", which it never created, so the SyntaxError it asserts could have come from that missing mark rather than from the null end mark it is about. The shared suite moves to 0baab7c, which measures the timeOrigin anchor as a min-of-N offset against a loose bound: reconstructing Date.now() from timeOrigin + now() races three clock reads, and a stall between them read as an anchoring error on a contended host.
| Back | FazBrowse Home | New Git URL |
Description
Replaces the bare native {now, timeOrigin} object with a spec-shaped implementation of hr-time, User Timing Level 3 and the performance timeline with PerformanceObserver. performance, Performance, PerformanceEntry, PerformanceMark, PerformanceMeasure, PerformanceObserver and PerformanceObserverEntryList are globals in main and worker isolates alike, with Performance extends EventTarget and WebIDL-shaped descriptors/brands.
Stacked on #2000 — review only the last two commits.
Architecture
Frame callbacks are the first consumer (2nd commit)
The clock hook exists so JS-visible timestamps share one base, so this PR wires up the runtime's existing frame callbacks rather than leaving the hook unused.
detail is structured-cloned
Building on #2000: mark/measure detail is cloned once at entry creation through the structuredClone global, so entries hold snapshots and an uncloneable detail throws the DataCloneError-named error. The builtin keeps an identity fallback so the file stays portable to a runtime shipping the Performance API before structuredClone. This is the Android side of the iOS follow-up commit 6dd55238d.
Deviations (documented in docs/performance.md)
Does your commit message include the wording below to reference a specific issue in this repo?
No — this is not tracked by an issue in this repo.
Related Pull Requests
Does your pull request have unit tests?
Yes. The cross-runtime shared suite (test-app/app/src/main/assets/app/shared/Performance, already at the pin this branch inherits) is registered from mainpage.js, contributing 56 specs — hr-time invariants, the full measure() options algebra, the timeline queries, observer semantics including buffered replay and takeRecords, detail snapshotting plus the DataCloneError case, and worker time-origin/buffer isolation. The suite gates itself on the API being present, so an unguarded Performance API canary was added to testRuntimeImplementedAPIs.js (mirroring iOS) to make a regression fail rather than silently skip. testPerformanceNow.js is removed — the shared suite supersedes it, as on iOS.
The frame-callback contract is covered by 4 new specs in testPostFrameCallback.js, run twice over — once per implementation — asserting the raw first argument stays boot-scale nanoseconds, that the second argument sits on the performance base (never ahead of a performance.now() read taken inside the callback, within a frame of it), that both advance across consecutive frames, and that the origin the two arguments imply matches the one (System.nanoTime(), performance.now()) implies. The results XML confirms the Java implementation suite ran with 2 tests / 0 skipped / 0 failures, so the API 21–23 bridge is device-tested rather than review-only.
Verified on an arm64-v8a API 35 emulator:
(+55 = 56 shared Performance specs + 1 canary − 1 self-skip gate spec − 2 superseded testPerformanceNow specs; +4 = the frame-callback specs × 2 implementations.) check_if_tests_passed.js on the pulled results XML: 749 executed, 749 passed, 0 failed. ESLint clean (npm run lint).
Summary by CodeRabbit
New Features
Documentation
Tests