perf(node): implement getOwnNonIndexProperties in native code#31393
perf(node): implement getOwnNonIndexProperties in native code#31393nathanwhit merged 2 commits intodenoland:mainfrom
Conversation
WalkthroughThis change adds a native op Sequence Diagram(s)sequenceDiagram
participant TS as TypeScript polyfill
participant OP as op_node_get_own_non_index_properties (Rust)
participant V8 as V8 API
TS->>OP: call op_node_get_own_non_index_properties(obj, filter)
note right of OP `#D3E4CD`: construct v8::PropertyFilter from bitmask
OP->>V8: get_property_names(obj, SkipIndices, OwnOnly, filter)
alt success
V8-->>OP: v8::Array (property names)
OP-->>TS: v8::Array
else failure
V8-->>OP: error / None
OP-->>TS: JsErrorBox (type_error)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ext/node/lib.rs(1 hunks)ext/node/ops/util.rs(1 hunks)ext/node/polyfills/internal_binding/util.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
ext/**/lib.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Extensions should provide ops (operations) exposed to JavaScript in Rust code within
ext/<extension_name>/directories
Files:
ext/node/lib.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
ext/node/lib.rsext/node/ops/util.rs
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
ext/node/polyfills/internal_binding/util.ts
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to ext/**/lib.rs : Extensions should provide ops (operations) exposed to JavaScript in Rust code within `ext/<extension_name>/` directories
Applied to files:
ext/node/lib.rsext/node/ops/util.rs
📚 Learning: 2025-11-20T09:12:20.198Z
Learnt from: marvinhagemeister
Repo: denoland/deno PR: 31358
File: ext/node/polyfills/process.ts:759-766
Timestamp: 2025-11-20T09:12:20.198Z
Learning: In ext/node/polyfills/process.ts, the process.sourceMapsEnabled property should be defined with enumerable: true to match Node.js behavior (as seen in Node.js source: lib/internal/bootstrap/node.js).
Applied to files:
ext/node/polyfills/internal_binding/util.ts
🧬 Code graph analysis (3)
ext/node/lib.rs (1)
ext/node/ops/util.rs (1)
op_node_get_own_non_index_properties(181-214)
ext/node/ops/util.rs (2)
ext/node/ops/v8.rs (1)
obj(60-65)ext/node/polyfills/internal_binding/util.ts (6)
ALL_PROPERTIES(43-43)ONLY_WRITABLE(44-44)ONLY_ENUMERABLE(45-45)ONLY_CONFIGURABLE(46-46)SKIP_STRINGS(48-48)SKIP_SYMBOLS(49-49)
ext/node/polyfills/internal_binding/util.ts (2)
ext/node/ops/util.rs (1)
op_node_get_own_non_index_properties(181-214)ext/node/polyfills/internal/buffer.mjs (2)
obj(790-790)filter(789-789)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug macos-x86_64
🔇 Additional comments (2)
ext/node/polyfills/internal_binding/util.ts (1)
32-32: LGTM!Clean delegation to the native op. The filter constants are correctly aligned with the Rust implementation.
Also applies to: 97-97
ext/node/lib.rs (1)
363-363: LGTM!The op is correctly registered in the extension. Placement alongside other util ops is appropriate.
This speeds up assertions on typed arrays by a ton. This unblocks one of
the node compat tests for advanced serialization, which times out
because the assertion is too slow.
Bench:
```
import assert from "node:assert";
const iters = 10;
let total = 0;
for (let i = 0; i < iters; i++) {
const start = performance.now();
assert.deepStrictEqual(
new Uint8Array(8 * 1024 * 1024).fill(0),
new Uint8Array(8 * 1024 * 1024).fill(0),
);
const end = performance.now();
total += end - start;
}
console.log("took", total / iters, "per iter");
```
Before:
```
took 2634.9984669 per iter
```
After:
```
took 15.0644873 per iter
```
So over 100x faster
This speeds up assertions on typed arrays by a ton. This unblocks one of the node compat tests for advanced serialization, which times out because the assertion is too slow.
Bench:
Before:
After:
So over 100x faster