GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat: WHATWG performance API (hr-time, user timing, performance timeline) by edusperoni · Pull Request #2001 · NativeScript/android Conversation
|
| specs | failures | |
|---|---|---|
base (feat/structured-clone, 975e724) |
701 | 0 |
| after the Performance API commit | 756 | 0 |
| this branch | 760 | 0 |
(+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).
New Features
performance.now() and timeOrigin for consistent runtime timestamps.Documentation
Tests
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe 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)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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.
67dd91e to
479dd74
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
test-app/runtime/src/main/cpp/Performance.cpp (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead 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
void Performance::Init(Local<Context> context) { - Isolate* isolate = Isolate::GetCurrent(); + Isolate* isolate = context->GetIsolate();🤖 Prompt for AI Agents
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.test-app/runtime/src/main/cpp/js/performance.js (1)
599-605: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInstall the globals as non-enumerable properties.
Plain assignment makes these properties enumerable. WebIDL defines interface objects and the
performanceattribute as non-enumerable, writable, and configurable. The rest of this file already reproduces the WebIDL shape throughfinishInterface, so the globals should match.♻️ Proposed refactor for the global installation
-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));🤖 Prompt for AI Agents
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.docs/performance.md (1)
78-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the debug-only
__setFrameCallbackImploverride.
FrameCallbacks::Initinstalls__setFrameCallbackImplunderAPPLICATION_IN_DEBUG, andtestPostFrameCallback.jsdepends 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 Agents
Verify 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
docs/README.mddocs/performance.mdeslint.config.mjstest-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/testPerformanceNow.jstest-app/app/src/main/assets/app/tests/testPostFrameCallback.jstest-app/app/src/main/assets/app/tests/testRuntimeImplementedAPIs.jstest-app/runtime/CMakeLists.txttest-app/runtime/src/main/cpp/CallbackHandlers.cpptest-app/runtime/src/main/cpp/CallbackHandlers.htest-app/runtime/src/main/cpp/FrameCallbacks.cpptest-app/runtime/src/main/cpp/FrameCallbacks.htest-app/runtime/src/main/cpp/Performance.cpptest-app/runtime/src/main/cpp/Performance.htest-app/runtime/src/main/cpp/Runtime.cpptest-app/runtime/src/main/cpp/Runtime.htest-app/runtime/src/main/cpp/js/performance.jstest-app/runtime/src/main/cpp/js/primordials.jstest-app/runtime/src/main/java/com/tns/FrameCallbacks.java
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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 `@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
docs/performance.mdtest-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/testPerformance.jstest-app/app/src/main/assets/app/tests/testPostFrameCallback.jstest-app/runtime/src/main/cpp/FrameCallbacks.cpptest-app/runtime/src/main/cpp/js/performance.jstest-app/runtime/src/main/java/com/tns/FrameCallbacks.java
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.