refactor: move embedded runtime JS to real .js files (js2c) + enum eval caching - #411
refactor: move embedded runtime JS to real .js files (js2c) + enum eval caching#411edusperoni wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughRuntime JavaScript implementations are moved into generated C++ builtin sources. A cached ChangesRuntime builtin generation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime
participant BuiltinLoader
participant V8
participant JavaScriptBuiltin
Runtime->>BuiltinLoader: RunBuiltin(context, builtinId)
BuiltinLoader->>V8: Consume cached code or compile source
V8->>JavaScriptBuiltin: Execute builtin
JavaScriptBuiltin-->>V8: Return initialization value
V8-->>BuiltinLoader: Return value
BuiltinLoader-->>Runtime: Provide function or dispatchers
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
…js2c The ~26KB of JavaScript previously embedded as C++ string literals now lives in NativeScript/runtime/js/*.js. A "Generate RuntimeBuiltins" build phase runs tools/js2c.mjs to emit a generated source table, and call sites go through BuiltinLoader::RunBuiltin, which sets proper internal/<name>.js script origins and shares an in-process bytecode cache across isolates so workers stop re-parsing the builtins.
Interop::WriteValue recompiled and re-ran an enum wrapper's __tsEnum snippet on every FFI marshal; the value is now memoized on the EnumDataWrapper. GlobalPropertyGetter did the same on every read of a JsCode global; the evaluated result is now defined as a real own property on the global (the interceptor is kNonMasking, so later reads bypass it entirely), with a reentrancy guard because CreateDataProperty re-enters the interceptor before the property exists.
958421c to
885abe9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tools/js2c-inputs.xcfilelist (1)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew builtin
.jsfiles must be added here too, or incremental builds silently skip regeneration.This file drives Xcode's incremental-build change detection for the "Generate RuntimeBuiltins" phase, but the phase's shell script actually globs
NativeScript/runtime/js/*.jsat runtime. If a contributor adds a new builtin script without updating this list, Xcode won't detect it as a changed input and may skip re-runningjs2c.mjs, leaving the new file out of the generatedRuntimeBuiltins.{h,cpp}until an unrelated input changes or a clean build happens.Consider adding a short comment here (or in
tools/js2c.mjs's usage text) noting that this list must be kept in sync withNativeScript/runtime/js/*.js.🤖 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 `@tools/js2c-inputs.xcfilelist` around lines 1 - 12, Add a concise maintenance comment to the input list documenting that every new NativeScript/runtime/js/*.js builtin must also be added here so Xcode incremental builds rerun js2c.mjs. Keep the existing file entries and generation behavior unchanged.NativeScript/runtime/js/require-factory.js (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: tidy the formatting carried over from the C++ string literal.
The stray line break inside the
ifbody (Lines 4-5) and trailing whitespace are artifacts of the old embedded literal. Now that this is a real source file, normalizing it costs nothing and keeps future diffs readable. Semantics unchanged.♻️ Proposed cleanup
-(function() { - function require_factory(requireInternal, dirName) { - return function require(modulePath) { - if(global.__pauseOnNextRequire) { debugger; -global.__pauseOnNextRequire = false; } - return requireInternal(modulePath, dirName); - } - } - return require_factory; -})() +(function () { + function require_factory(requireInternal, dirName) { + return function require(modulePath) { + if (global.__pauseOnNextRequire) { + debugger; + global.__pauseOnNextRequire = false; + } + return requireInternal(modulePath, dirName); + }; + } + return require_factory; +})();🤖 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 `@NativeScript/runtime/js/require-factory.js` around lines 1 - 10, Normalize formatting in the require_factory wrapper, especially the __pauseOnNextRequire conditional, by removing the embedded line break and trailing whitespace while preserving debugger invocation, flag reset, and requireInternal behavior.NativeScript/runtime/js/smart-stringify.js (1)
3-13: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional:
seenas an array yields O(n²) scans and false[Circular]hits for shared (non-cyclic) references.
seen.indexOf(value)is linear per visited object, and entries are never popped when leaving a subtree, so a repeated-but-acyclic reference is reported as circular. Swapping to aSetfixes the complexity; fixing the false positives needs an ancestor stack instead. Both change existing output, so this is fine to defer given the PR's extraction-fidelity goal.🤖 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 `@NativeScript/runtime/js/smart-stringify.js` around lines 3 - 13, Defer changes to the circular-reference tracking in the replacer function: retain the existing seen array and output behavior for this extraction-focused change. Do not alter complexity or shared-reference handling unless a follow-up explicitly scopes those behavioral changes.NativeScript/runtime/Helpers.mm (1)
497-503: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a
TryCatcharound the builtin run.If
RunBuiltinfails (compile/run error), the scheduled exception stays pending on the isolate while this function returns an empty handle.JsonStringifyObjectthen continues intoJSON::Stringify, so the failure surfaces at an unrelated site. Wrapping this call in a localTryCatch(and logging viatns::LogError) keeps the failure contained, matching the pattern used elsewhere in this file.♻️ Proposed refactor
Local<Value> result; - bool success = BuiltinLoader::RunBuiltin(context, BuiltinId::kSmartStringify).ToLocal(&result); - tns::Assert(success, isolate); - - if (result.IsEmpty() || !result->IsFunction()) { + TryCatch tc(isolate); + bool success = BuiltinLoader::RunBuiltin(context, BuiltinId::kSmartStringify).ToLocal(&result); + if (!success || tc.HasCaught()) { + tns::LogError(isolate, tc); + return Local<v8::Function>(); + } + + if (result.IsEmpty() || !result->IsFunction()) { return Local<v8::Function>(); }🤖 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 `@NativeScript/runtime/Helpers.mm` around lines 497 - 503, In the builtin-loading flow around BuiltinLoader::RunBuiltin, add a local v8::TryCatch and handle compile/run failures before returning an empty function handle. Log the caught exception through tns::LogError, clear or contain the pending isolate exception, and preserve the existing result.IsEmpty()/IsFunction() validation for successful execution.NativeScript/runtime/WeakRef.cpp (1)
10-19: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider capturing exception details before asserting, like
TSHelpers::Init.Unlike
TSHelpers::Init(which wraps the call in aTryCatchand callstns::LogError(isolate, tc)before asserting), this path asserts on baresuccesswith no way to see why the WeakRef builtin failed to compile/run. Since WeakRef is treated as similarly critical (hardAssert), aligning the diagnostics would make a future failure easier to debug.♻️ Proposed alignment with TSHelpers::Init
void WeakRef::Init(Local<Context> context) { Isolate* isolate = v8::Isolate::GetCurrent(); + TryCatch tc(isolate); Local<Value> result; - bool success = - BuiltinLoader::RunBuiltin(context, BuiltinId::kWeakRef).ToLocal(&result); - tns::Assert(success, isolate); + if (!BuiltinLoader::RunBuiltin(context, BuiltinId::kWeakRef).ToLocal(&result) && + tc.HasCaught()) { + tns::LogError(isolate, tc); + } + tns::Assert(!result.IsEmpty(), isolate); }🤖 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 `@NativeScript/runtime/WeakRef.cpp` around lines 10 - 19, Update WeakRef::Init to wrap BuiltinLoader::RunBuiltin in a v8::TryCatch, log the captured exception with tns::LogError(isolate, tc) when execution fails, then retain the existing tns::Assert(success, isolate) behavior.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@NativeScript/runtime/Interop.mm`:
- Around line 630-635: Update the enum evaluation flow around
script->Run(context) so a failed ToLocal conversion is asserted or handled
before result is dereferenced. Remove the result.IsEmpty() condition that
suppresses failure handling, and ensure result->IsNumber() is reached only when
result contains a valid value.
---
Nitpick comments:
In `@NativeScript/runtime/Helpers.mm`:
- Around line 497-503: In the builtin-loading flow around
BuiltinLoader::RunBuiltin, add a local v8::TryCatch and handle compile/run
failures before returning an empty function handle. Log the caught exception
through tns::LogError, clear or contain the pending isolate exception, and
preserve the existing result.IsEmpty()/IsFunction() validation for successful
execution.
In `@NativeScript/runtime/js/require-factory.js`:
- Around line 1-10: Normalize formatting in the require_factory wrapper,
especially the __pauseOnNextRequire conditional, by removing the embedded line
break and trailing whitespace while preserving debugger invocation, flag reset,
and requireInternal behavior.
In `@NativeScript/runtime/js/smart-stringify.js`:
- Around line 3-13: Defer changes to the circular-reference tracking in the
replacer function: retain the existing seen array and output behavior for this
extraction-focused change. Do not alter complexity or shared-reference handling
unless a follow-up explicitly scopes those behavioral changes.
In `@NativeScript/runtime/WeakRef.cpp`:
- Around line 10-19: Update WeakRef::Init to wrap BuiltinLoader::RunBuiltin in a
v8::TryCatch, log the captured exception with tns::LogError(isolate, tc) when
execution fails, then retain the existing tns::Assert(success, isolate)
behavior.
In `@tools/js2c-inputs.xcfilelist`:
- Around line 1-12: Add a concise maintenance comment to the input list
documenting that every new NativeScript/runtime/js/*.js builtin must also be
added here so Xcode incremental builds rerun js2c.mjs. Keep the existing file
entries and generation behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 28ac2389-bd4d-4691-9d9f-00fe74a3aa30
📒 Files selected for processing (32)
.gitignoreNativeScript/runtime/BuiltinLoader.cppNativeScript/runtime/BuiltinLoader.hNativeScript/runtime/ClassBuilder.mmNativeScript/runtime/DataWrapper.hNativeScript/runtime/ErrorEvents.cppNativeScript/runtime/Events.cppNativeScript/runtime/Helpers.mmNativeScript/runtime/InlineFunctions.cppNativeScript/runtime/Interop.mmNativeScript/runtime/MetadataBuilder.hNativeScript/runtime/MetadataBuilder.mmNativeScript/runtime/ModuleInternal.mmNativeScript/runtime/PromiseProxy.cppNativeScript/runtime/Runtime.mmNativeScript/runtime/TSHelpers.cppNativeScript/runtime/WeakRef.cppNativeScript/runtime/js/blob-url.jsNativeScript/runtime/js/class-extends.jsNativeScript/runtime/js/error-events.jsNativeScript/runtime/js/events.jsNativeScript/runtime/js/inline-functions.jsNativeScript/runtime/js/promise-proxy.jsNativeScript/runtime/js/require-factory.jsNativeScript/runtime/js/smart-stringify.jsNativeScript/runtime/js/ts-helpers.jsNativeScript/runtime/js/weak-ref.jspackage.jsontools/js2c-inputs.xcfilelisttools/js2c-outputs.xcfilelisttools/js2c.mjsv8ios.xcodeproj/project.pbxproj
…rameter
Adopts Node's internals idiom: every builtin is compiled via
ScriptCompiler::CompileFunction with the single parameter `binding`, and
natives arrive as properties of one bag object that each file
destructures at the top (const { isRuntimeRunloop } = binding). The
visible IIFE wrappers are gone, top-level return is the way a builtin
hands a value back to C++, and the bytecode cache moves to
CreateCodeCacheForFunction.
The parameter name is fixed and hardcoded in BuiltinLoader, so there is
no per-builtin name to mistype in C++; on the JS side an ESLint gate
(no-undef with `binding` and the reachable native globals declared)
catches typos in the destructures, wired into lint-staged. Conventions
are documented in NativeScript/runtime/js/README.md.
b4fa9d7 to
434c1c7
Compare
The enum-evaluation branch in Interop::WriteValue could dereference an empty handle when Run failed (the && !result.IsEmpty() condition suppressed the assert); WeakRef::Init and GetSmartJSONStringifyFunction now log the caught exception before asserting so a builtin failure is diagnosable; js2c gains --filelist, wired into the build phase, so a builtin missing from js2c-inputs.xcfilelist fails the build instead of being silently skipped by incremental change detection.
|
Review feedback disposition (a56b0d4):
Suite after fixes: 905/905 on the V8 14 base. |
2 participants
What
Moves the ~26KB of JavaScript that was embedded as C++ string literals into real, version-controlled
.jsfiles underNativeScript/runtime/js/, compiled into the framework at build time (Node-style js2c), and adds two steady-state perf fixes for jitless mode.Rebased onto
mainafter the V8 14.9.207.39 upgrade (#412); originally stacked on #409.Extraction & build
tools/js2c.mjs(Node CLI) convertsruntime/js/*.jsinto a generatedRuntimeBuiltins.{h,cpp}table (byte arrays, deterministic output). Supports--minifyvia esbuild (whitespace+syntax only) — implemented but not enabled in any configuration.NativeScript/runtime/generated/is gitignored.mainafter the V8-upgrade reformatting.Loader
ScriptCompiler::CompileFunctionwith one fixed parameter,binding— natives arrive as properties of a bag object built by the C++ call site and each file destructures what it needs (const { isRuntimeRunloop } = binding;). No IIFE wrappers; top-levelreturnis how a builtin hands a value back to C++. Conventions are documented inNativeScript/runtime/js/README.md.BuiltinLoader::RunBuiltincompiles with a properinternal/<name>.jsscript origin (runtime frames are identifiable in stack traces) and a process-wide bytecode cache — first isolate compiles withkEagerCompile+CreateCodeCacheForFunction, workers/subsequent isolates consume viakConsumeCodeCache(with rejected-cache fallback), so the builtins are parsed once per process instead of once per isolate.no-undefwithbindingand the reachable native globals declared, wired into lint-staged) catches typos in the destructures.Perf (jitless steady-state)
Interop::WriteValueno longer recompiles + re-runs an enum's__tsEnumsnippet on every FFI marshal — the numeric value is memoized onEnumDataWrapper.GlobalPropertyGetterno longer recompiles a JsCode global's snippet on every read — the evaluated result is defined as a real own property on the global (interceptor iskNonMasking, so later reads bypass it entirely). Includes a reentrancy guard:CreateDataPropertyre-enters the interceptor before the property exists.Behavior notes
internal/<name>.jsorigins instead of anonymous<eval>frames.mainremovedCreatePlaceholderModuleentirely (missing modules throwCannot find module), which matches the de-facto behavior this PR had preserved.Testing
main(V8 14.9.207.39): 905 tests, 0 failures (includes worker tests, which exercise the cross-isolate code cache)..jsinput is touched.