-
Notifications
You must be signed in to change notification settings - Fork 903
Implement ES2021 WeakRef and FinalizationRegistry #2058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
ab4f5a2 to
67a3ea1
Compare
gbrail
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks -- this looks like good progress. I have a bunch of questions, please take a look -- thanks!
rhino/src/main/java/org/mozilla/javascript/NativeFinalizationRegistry.java
Outdated
Show resolved
Hide resolved
rhino/src/main/java/org/mozilla/javascript/NativeFinalizationRegistry.java
Outdated
Show resolved
Hide resolved
rhino/src/main/java/org/mozilla/javascript/NativeFinalizationRegistry.java
Outdated
Show resolved
Hide resolved
rhino/src/main/java/org/mozilla/javascript/NativeFinalizationRegistry.java
Outdated
Show resolved
Hide resolved
rhino/src/main/java/org/mozilla/javascript/NativeFinalizationRegistry.java
Outdated
Show resolved
Hide resolved
rhino/src/main/java/org/mozilla/javascript/NativeFinalizationRegistry.java
Outdated
Show resolved
Hide resolved
rhino/src/main/java/org/mozilla/javascript/NativeFinalizationRegistry.java
Outdated
Show resolved
Hide resolved
rhino/src/main/java/org/mozilla/javascript/NativeFinalizationRegistry.java
Outdated
Show resolved
Hide resolved
11cb533 to
b10db34
Compare
cdbeab4 to
c0a925d
Compare
aardvark179
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I'd written a review of this, but I must have closed the tab or something.
So, according to the spec the cleanup routines should be run when the weak ref target is no EMPTY, so this should probably be written using PhantomReferences (they are put on the queue when their referent has been collected). I am also unsure what the effect is on the JVM of create a large number of processor queues, and they can be tricky to manage correctly.
Could you try implementing this using a single queue (maybe using a java.lang.ref.Cleaner and putting the tasks from there onto a queue in each Context. We could likely combine processing of those with the micro task loop run as the context is exited.
I think that would help avoid large reference queues hanging around, and would make it easier to understand when things can be run.
acc97e5 to
ae802e2
Compare
|
@aardvark179 Thank you for the detailed review! Yes, he did leave inline comments earlier and I responded to them and followed the suggestions. However, after refactoring the implementation, the filenames changed and I can't access those same comments anymore. I understand your architectural suggestions:
|
…ferences Complete rewrite addressing all PR mozilla#2058 feedback from aardvark179: **Architecture Changes:** - Replace WeakReference with PhantomReference for correct GC semantics - Implement singleton FinalizationQueueManager with shared ReferenceQueue - Add Context integration for proper JavaScript execution timing - Thread-safe operations with ConcurrentHashMap and synchronized blocks **Key Improvements:** - Memory leak prevention with proper WeakReference usage in TokenKey - O(1) token-based unregistration with reverse index - Bounded cleanup queue (MAX_PENDING_CLEANUPS=10000) prevents OOM - Complete cleanupSome() implementation with callback override support - Comprehensive error handling with thread safety **New Components:** - FinalizationQueueManager.java - Singleton Cleaner-like infrastructure - FinalizationRegistryCleanupBehaviorTest.java - 10 cleanup behavior tests - FinalizationRegistryCrossRealmTest.java - 6 cross-realm scenarios **Test Coverage:** - 72+ tests across 5 test files - Cross-realm support validated - Internal implementation white-box testing - GC integration verified Ready for PR submission to Mozilla Rhino.
|
@aardvark179 Implementation completely rewritten per your feedback: PhantomReferences now replace WeakReferences for correct GC semantics. Added shared ReferenceQueue with Context integration for cleanup processing in the JavaScript execution loop as requested. Key improvements:
Ready for re-review! |
…ferences Complete rewrite addressing all PR mozilla#2058 feedback from aardvark179: **Architecture Changes:** - Replace WeakReference with PhantomReference for correct GC semantics - Implement singleton FinalizationQueueManager with shared ReferenceQueue - Add Context integration for proper JavaScript execution timing - Thread-safe operations with ConcurrentHashMap and synchronized blocks **Key Improvements:** - Memory leak prevention with proper WeakReference usage in TokenKey - O(1) token-based unregistration with reverse index - Bounded cleanup queue (MAX_PENDING_CLEANUPS=10000) prevents OOM - Complete cleanupSome() implementation with callback override support - Comprehensive error handling with thread safety **New Components:** - FinalizationQueueManager.java - Singleton Cleaner-like infrastructure - FinalizationRegistryCleanupBehaviorTest.java - 10 cleanup behavior tests - FinalizationRegistryCrossRealmTest.java - 6 cross-realm scenarios **Test Coverage:** - 72+ tests across 5 test files - Cross-realm support validated - Internal implementation white-box testing - GC integration verified Ready for PR submission to Mozilla Rhino.
ac0dde3 to
fd1cb89
Compare
|
Sorry I haven’t got to re-reviewing this one yet. It’s been a busy few weeks, but I promise I will get to it. It would also be great to have a chat at some point so I can understand your Rhino use case better and maybe help plan out which features are the best ones to tackle first to enable that. |
97f1921 to
5d25301
Compare
9c07c68 to
db27c5f
Compare
|
No worries @aardvark179! I'd be interested in that chat about Rhino use cases and feature prioritization. Feel free to reach out whenever works for your schedule. Thanks for the architectural guidance on PhantomReference and shared queue - it made the implementation much cleaner. |
…compliance Complete rewrite using simplified, Android-compatible approach. Changes: - NativeWeakRef: ES2021-compliant WeakRef implementation - NativeFinalizationRegistry: ES2021-compliant implementation - ScriptRuntime: Register both constructors in ES6+ block - Messages.properties: Add validation error messages - test262.properties: All 76 tests passing ES2021 Standard Compliance: - Correctly allows unregistered symbols as weak targets - Rejects registered symbols created with Symbol.for per spec - Validates target/heldValue/token same-value constraints - Proper Symbol.toStringTag on prototypes - Error messages match spec requirements Technical Implementation: - Uses Java WeakReference for WeakRef, simple and direct - Uses PhantomReference + ReferenceQueue for FinalizationRegistry - No Cleaner API dependency, not available on Android - No Context.java modifications needed - Thread-safe with ConcurrentHashMap - Cleanup callbacks processed opportunistically during register calls - Follows modern LambdaConstructor pattern like NativeWeakMap Benefits: - Only 2 files instead of 5 - Self-contained, no helper classes or separate queue management - Android-compatible using Java 8 APIs only - Addresses reviewer feedback to use simpler, standard Java APIs
c45e8c7 to
6e9456a
Compare
|
Force pushed a simplified implementation that follows the NativeWeakMap pattern. All tests passing, Android-compatible, no Context modifications needed. Ready for review. |
Implements ES2021 WeakRef and FinalizationRegistry following the NativeWeakMap pattern.
Implementation
Two self-contained classes, about 400 lines total:
The finalization approach is simple: poll the ReferenceQueue during register() calls to process any pending cleanups. No separate queue management or Context modifications needed.
Android Compatibility
Uses PhantomReference instead of the Cleaner API since Cleaner isn't available on Android. This keeps the implementation portable across all Java 8+ platforms.
ES2021 Standard Compliance
Correctly implements symbol handling per spec:
Test Results
All 76 test262 tests passing:
Also fixes 2 Object.seal tests.
Thread-safe implementation using ConcurrentHashMap for registration tracking.