Repository navigation
Release Hermes WeakRef targets after draining jobs - #262
Merged
bkaradzic-microsoft merged 3 commits intoOct 6, 2026
Merged
bkaradzic-microsoft merged 3 commits into
bkaradzic-microsoft merged 3 commits into
Conversation
Clear the runtime's kept-objects set after its microtask queue drains, matching the Hermes JSI and ABI embeddings. Otherwise WeakRef targets remain runtime roots across turns and retain disposed application graphs. Add a bounded native regression that retains 256 MiB without the fix and less than 64 MiB with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The regression test references a Hermes-only API while being compiled for every backend, breaking non-Hermes unit-test builds.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds Hermes WeakRef cleanup after job draining to prevent targets remaining rooted across JavaScript turns.
Changes:
- Calls
clearKeptObjects()after draining Hermes jobs. - Adds a regression test tracking retained external bytes.
| File | Description |
|---|---|
Core/Node-API/Source/env_hermes.cc |
Releases Hermes-kept WeakRef targets after job drains. |
Tests/UnitTests/Source/Tests.NodeApi.cpp |
Adds the Hermes memory-retention regression test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Move the test into the existing engine-specific source selection. A runtime skip cannot hide the Hermes-only DrainJobs API from other engines' compilers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Use performance.now() instead of Date timestamps for elapsed timer checks. This avoids x86 Chakra's wall-clock precision causing a 29 ms observation against a 30 ms interval bound. Keep all existing minimum delays unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
bkaradzic-microsoft
added a commit
to BabylonJS/BabylonNative
that referenced
this pull request
Oct 7, 2026
## Summary Improve native rendering parity with **Babylon.js 9.29.0 / protocol 9**: - Fix instancing, shader coordinates/varyings, texture sampling/readback, MSAA, and framebuffer handling. - Fix Canvas paths/clipping, image lifetime/I/O, text metrics, alpha representations, and mipmaps. - Improve validation runtime support and enable **57 additional scenes: 314 -> 371**, with **349 excluded** out of 720. No upstream-enabled scenes are disabled. Reference images, comparison thresholds, and existing backend exclusions are unchanged. **16 commits:** 14 code commits, one generated JavaScript bundle update, and one catalog update. Compute shaders, storage/dispatch changes, GPU instance repacking/topology expansion, protocol 10, and paired Babylon.js changes remain excluded. ## Current status - **Review:** latest integer-coordinate, explicit-gradient, and interface-block concerns addressed. Maintainer approval is recorded. - **CI:** [rerunning for `e5ec2268`](https://github.com/BabylonJS/BabylonNative/actions/runs/37551896463). - **Focused validation:** 30/30 cases pass with WEBMIN disabled; 27 pass with default WEBMIN, with three interface-block skips; 2/2 DXIL compilation cases pass. - **Earlier full validation:** uninstrumented Hermes passed 371/371 scenes at the unchanged 512 MiB heap limit; full Hermes and Chakra/ASan native suites each passed 123/123. - **Dependency:** upstream JsRuntimeHost `2f8b9be3` includes the merged Hermes WeakRef fix from BabylonJS/JsRuntimeHost#262. No temporary runtime fork remains; all other dependency pins are unchanged. Before landing: finish refreshed CI. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
bkaradzic-microsoft
added a commit
to bkaradzic-microsoft/JsRuntimeHost
that referenced
this pull request
Oct 7, 2026
Replay the remaining V8 and Apple JavaScriptCore rejection tracking over upstream BabylonJS#262, preserving the merged Node-API lifetime and test changes. Keep same-turn handled promises out of the host error handler and wire the existing feature-specific regressions into the reorganized tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
bkaradzic-microsoft
added a commit
to bkaradzic-microsoft/JsRuntimeHost
that referenced
this pull request
Oct 7, 2026
Replay the unmerged successful-load event and asynchronous URL-open failure changes over upstream BabylonJS#262. Both remain necessary for the procedural texture and clustered-light serialization scenes. Preserve the upstream binary response fix and reorganized test layout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix
Call
Runtime::clearKeptObjects()after draining Hermes microtasks, matching Hermes's own JSI and ABI embeddings. Without this, targets protected byWeakRefremain rooted across JavaScript turns, even after full GC.This was identified while investigating retained disposed scenes and heap exhaustion in BabylonJS/BabylonNative#1913. It does not increase the heap limit.
Regression
The new Hermes-only native test creates 256 one-MiB weak targets across job drains. The original embedding retains 268,437,568 external bytes and fails the 64 MiB bound; the fixed embedding passes.
The regression is selected by CMake only for Hermes, alongside the existing engine-specific Node-API sources. Shared tests remain unchanged; other engines do not compile a reference to the Hermes-only
DrainJobsAPI.Windows Hermes native Node-API/runtime/scheduling coverage: 10 passed, one existing engine-specific skip. BabylonNative production validation with this runtime fix passes 123/123 native tests and 371/371 rendering scenes, with snapshots enabled, memory instrumentation disabled, stock Babylon.js 9.29.0, and the unchanged 512 MiB heap limit. Follow-up CI is still required.
CI follow-up
Non-Hermes compilation is repaired by selecting the regression at build time, not with a runtime skip. JSI builds and passes its applicable runtime tests.
The next run exposed an existing x86 Chakra scheduling assertion using
Date(29 ms observed against a 30 ms bound). Timer tests now use the high-resolutionperformance.now()clock, with unchanged minimum-delay assertions and no scheduling implementation changes. The complete x86 Chakra suite passes 26/26, including 251 JavaScript cases, in each of three consecutive iterations. Fresh CI remains required.