From 77c88ff4e3cffab6caac4a5f7c2d480c5454b4c6 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Tue, 28 Jan 2025 13:51:24 +0200 Subject: [PATCH 1/6] Fix typos in JavaDoc comments --- .../java/org/truffleruby/collections/ConcurrentWeakKeysMap.java | 2 +- .../org/truffleruby/core/hash/CompareByRubyIdentityWrapper.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java b/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java index bbc201c107a..dbaee24e294 100644 --- a/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java +++ b/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java @@ -18,7 +18,7 @@ import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; -/** A concurrent thread-safe map with weak keys. Keys cannot be null. This map currently assumes keys do no depend on +/** A concurrent thread-safe map with weak keys. Keys cannot be null. This map currently assumes keys do not depend on * Hashing and so there is no need for ReHashable. So far this is only used for keys which are compared by identity. */ public class ConcurrentWeakKeysMap { diff --git a/src/main/java/org/truffleruby/core/hash/CompareByRubyIdentityWrapper.java b/src/main/java/org/truffleruby/core/hash/CompareByRubyIdentityWrapper.java index 729da43e8db..b7baab53ddc 100644 --- a/src/main/java/org/truffleruby/core/hash/CompareByRubyIdentityWrapper.java +++ b/src/main/java/org/truffleruby/core/hash/CompareByRubyIdentityWrapper.java @@ -12,7 +12,7 @@ import org.truffleruby.core.basicobject.ReferenceEqualNode; import org.truffleruby.core.hash.HashingNodes.ToHashByIdentity; -/** Wraps a value so that it will compared and hashed according to Ruby identity semantics. These semantics differ from +/** Wraps a value so that it will be compared and hashed according to Ruby identity semantics. These semantics differ from * Java semantics notably for primitives (e.g. all the NaN are different according to Ruby, but Java compares them * equal). */ public final class CompareByRubyIdentityWrapper { From cd06103ec59adf42f5706d48ddf4d52a979b54d8 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Tue, 28 Jan 2025 22:55:28 +0200 Subject: [PATCH 2/6] Implement ObjectSpace::WeakKeyMap Co-authored-by: Benoit Daloze --- .../core/objectspace/weakkeymap/clear_spec.rb | 27 ++++ .../weakkeymap/element_reference_spec.rb | 79 +++++++++- .../weakkeymap/fixtures/classes.rb | 5 + .../objectspace/weakkeymap/delete_tags.txt | 4 - .../weakkeymap/element_reference_tags.txt | 3 +- .../weakkeymap/element_set_tags.txt | 3 - .../objectspace/weakkeymap/getkey_tags.txt | 1 - .../objectspace/weakkeymap/inspect_tags.txt | 1 - .../core/objectspace/weakkeymap/key_tags.txt | 3 - .../methods/ObjectSpace::WeakKeyMap.txt | 7 + spec/truffle/methods/ObjectSpace::WeakMap.txt | 15 ++ spec/truffle/methods_spec.rb | 3 +- .../java/org/truffleruby/RubyLanguage.java | 2 + .../truffleruby/builtins/BuiltinsClasses.java | 5 + .../collections/ConcurrentWeakKeysMap.java | 49 ++++-- .../collections/ConcurrentWeakSet.java | 5 + .../collections/WeakValueCache.java | 8 +- .../org/truffleruby/core/CoreLibrary.java | 3 + .../hash/CompareByRubyHashEqlWrapper.java | 62 ++++++++ .../hash/CompareByRubyIdentityWrapper.java | 13 +- .../truffleruby/core/hash/HashingNodes.java | 4 + .../truffleruby/core/kernel/KernelNodes.java | 11 -- .../core/objectspace/RubyWeakKeyMap.java | 24 +++ .../core/objectspace/RubyWeakMap.java | 5 +- .../core/objectspace/WeakKeyMapNodes.java | 148 ++++++++++++++++++ .../core/objectspace/WeakKeyMapStorage.java | 17 ++ .../core/objectspace/WeakMapNodes.java | 20 ++- .../core/objectspace/WeakMapStorage.java | 1 + src/main/ruby/truffleruby/core/weakkeymap.rb | 18 +++ src/main/ruby/truffleruby/core/weakmap.rb | 6 +- 30 files changed, 487 insertions(+), 65 deletions(-) create mode 100644 spec/ruby/core/objectspace/weakkeymap/clear_spec.rb create mode 100644 spec/ruby/core/objectspace/weakkeymap/fixtures/classes.rb delete mode 100644 spec/tags/core/objectspace/weakkeymap/delete_tags.txt delete mode 100644 spec/tags/core/objectspace/weakkeymap/getkey_tags.txt delete mode 100644 spec/tags/core/objectspace/weakkeymap/inspect_tags.txt delete mode 100644 spec/tags/core/objectspace/weakkeymap/key_tags.txt create mode 100644 spec/truffle/methods/ObjectSpace::WeakKeyMap.txt create mode 100644 spec/truffle/methods/ObjectSpace::WeakMap.txt create mode 100644 src/main/java/org/truffleruby/core/hash/CompareByRubyHashEqlWrapper.java create mode 100644 src/main/java/org/truffleruby/core/objectspace/RubyWeakKeyMap.java create mode 100644 src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java create mode 100644 src/main/java/org/truffleruby/core/objectspace/WeakKeyMapStorage.java create mode 100644 src/main/ruby/truffleruby/core/weakkeymap.rb diff --git a/spec/ruby/core/objectspace/weakkeymap/clear_spec.rb b/spec/ruby/core/objectspace/weakkeymap/clear_spec.rb new file mode 100644 index 00000000000..042c444e1fd --- /dev/null +++ b/spec/ruby/core/objectspace/weakkeymap/clear_spec.rb @@ -0,0 +1,27 @@ +require_relative '../../../spec_helper' + +ruby_version_is '3.3' do + describe "ObjectSpace::WeakKeyMap#clear" do + it "removes all the entries" do + m = ObjectSpace::WeakKeyMap.new + + key = Object.new + value = Object.new + m[key] = value + + key2 = Object.new + value2 = Object.new + m[key2] = value2 + + m.clear + + m.key?(key).should == false + m.key?(key2).should == false + end + + it "returns self" do + m = ObjectSpace::WeakKeyMap.new + m.clear.should.equal?(m) + end + end +end \ No newline at end of file diff --git a/spec/ruby/core/objectspace/weakkeymap/element_reference_spec.rb b/spec/ruby/core/objectspace/weakkeymap/element_reference_spec.rb index 862480cd02f..c91375f9f0d 100644 --- a/spec/ruby/core/objectspace/weakkeymap/element_reference_spec.rb +++ b/spec/ruby/core/objectspace/weakkeymap/element_reference_spec.rb @@ -1,4 +1,5 @@ require_relative '../../../spec_helper' +require_relative 'fixtures/classes' ruby_version_is "3.3" do describe "ObjectSpace::WeakKeyMap#[]" do @@ -15,12 +16,88 @@ map[key2].should == ref2 end - it "matches using equality semantics" do + it "compares keys with #eql? semantics" do + map = ObjectSpace::WeakKeyMap.new + map[[1.0]] = "x" + map[[1]].should == nil + map[[1.0]].should == "x" + + map = ObjectSpace::WeakKeyMap.new + map[[1]] = "x" + map[[1.0]].should == nil + map[[1]].should == "x" + map = ObjectSpace::WeakKeyMap.new key1, key2 = %w[a a].map(&:upcase) ref = "x" map[key1] = ref map[key2].should == ref end + + it "compares key via #hash first" do + x = mock('0') + x.should_receive(:hash).and_return(0) + + map = ObjectSpace::WeakKeyMap.new + map['foo'] = :bar + map[x].should == nil + end + + it "does not compare keys with different #hash values via #eql?" do + x = mock('x') + x.should_not_receive(:eql?) + x.stub!(:hash).and_return(0) + + y = mock('y') + y.should_not_receive(:eql?) + y.stub!(:hash).and_return(1) + + map = ObjectSpace::WeakKeyMap.new + map[y] = 1 + map[x].should == nil + end + + it "compares keys with the same #hash value via #eql?" do + x = mock('x') + x.should_receive(:eql?).and_return(true) + x.stub!(:hash).and_return(42) + + y = mock('y') + y.should_not_receive(:eql?) + y.stub!(:hash).and_return(42) + + map = ObjectSpace::WeakKeyMap.new + map[y] = 1 + map[x].should == 1 + end + + it "finds a value via an identical key even when its #eql? isn't reflexive" do + x = mock('x') + x.should_receive(:hash).at_least(1).and_return(42) + x.stub!(:eql?).and_return(false) # Stubbed for clarity and latitude in implementation; not actually sent by MRI. + + map = ObjectSpace::WeakKeyMap.new + map[x] = :x + map[x].should == :x + end + + it "supports keys with private #hash method" do + key = WeakKeyMapSpecs::KeyWithPrivateHash.new + map = ObjectSpace::WeakKeyMap.new + map[key] = 42 + map[key].should == 42 + end + + it "raises ArgumentError when numbers or Symbols are used as keys, that cannot be garbage collected" do + -> { + map = ObjectSpace::WeakKeyMap.new + map[1.0] = "x" + }.should raise_error(ArgumentError, "WeakKeyMap must be garbage collectable") + + -> { + map = ObjectSpace::WeakKeyMap.new + map[:a] = "x" + }.should raise_error(ArgumentError, "WeakKeyMap must be garbage collectable") + end end end diff --git a/spec/ruby/core/objectspace/weakkeymap/fixtures/classes.rb b/spec/ruby/core/objectspace/weakkeymap/fixtures/classes.rb new file mode 100644 index 00000000000..0fd04551b50 --- /dev/null +++ b/spec/ruby/core/objectspace/weakkeymap/fixtures/classes.rb @@ -0,0 +1,5 @@ +module WeakKeyMapSpecs + class KeyWithPrivateHash + private :hash + end +end diff --git a/spec/tags/core/objectspace/weakkeymap/delete_tags.txt b/spec/tags/core/objectspace/weakkeymap/delete_tags.txt deleted file mode 100644 index fa011c36c63..00000000000 --- a/spec/tags/core/objectspace/weakkeymap/delete_tags.txt +++ /dev/null @@ -1,4 +0,0 @@ -fails:ObjectSpace::WeakKeyMap#delete removes the entry and returns the deleted value -fails:ObjectSpace::WeakKeyMap#delete uses equality semantic -fails:ObjectSpace::WeakKeyMap#delete calls supplied block if the key is not found -fails:ObjectSpace::WeakKeyMap#delete returns nil if the key is not found when no block is given diff --git a/spec/tags/core/objectspace/weakkeymap/element_reference_tags.txt b/spec/tags/core/objectspace/weakkeymap/element_reference_tags.txt index db6095f6aaa..5b41b6fdcb1 100644 --- a/spec/tags/core/objectspace/weakkeymap/element_reference_tags.txt +++ b/spec/tags/core/objectspace/weakkeymap/element_reference_tags.txt @@ -1,2 +1 @@ -fails:ObjectSpace::WeakKeyMap#[] is faithful to the map's content -fails:ObjectSpace::WeakKeyMap#[] matches using equality semantics +fails:ObjectSpace::WeakKeyMap#[] raises ArgumentError when numbers or Symbols are used as keys, that cannot be garbage collected diff --git a/spec/tags/core/objectspace/weakkeymap/element_set_tags.txt b/spec/tags/core/objectspace/weakkeymap/element_set_tags.txt index 36ad794564a..c86bda7de68 100644 --- a/spec/tags/core/objectspace/weakkeymap/element_set_tags.txt +++ b/spec/tags/core/objectspace/weakkeymap/element_set_tags.txt @@ -1,6 +1,3 @@ -fails:ObjectSpace::WeakKeyMap#[]= is correct -fails:ObjectSpace::WeakKeyMap#[]= requires the keys to implement #hash -fails:ObjectSpace::WeakKeyMap#[]= accepts frozen keys or values fails:ObjectSpace::WeakKeyMap#[]= rejects symbols as keys fails:ObjectSpace::WeakKeyMap#[]= rejects integers as keys fails:ObjectSpace::WeakKeyMap#[]= rejects floats as keys diff --git a/spec/tags/core/objectspace/weakkeymap/getkey_tags.txt b/spec/tags/core/objectspace/weakkeymap/getkey_tags.txt deleted file mode 100644 index 7e0ef070b1a..00000000000 --- a/spec/tags/core/objectspace/weakkeymap/getkey_tags.txt +++ /dev/null @@ -1 +0,0 @@ -fails:ObjectSpace::WeakKeyMap#getkey returns the existing equal key diff --git a/spec/tags/core/objectspace/weakkeymap/inspect_tags.txt b/spec/tags/core/objectspace/weakkeymap/inspect_tags.txt deleted file mode 100644 index d685d1cf6a8..00000000000 --- a/spec/tags/core/objectspace/weakkeymap/inspect_tags.txt +++ /dev/null @@ -1 +0,0 @@ -fails:ObjectSpace::WeakKeyMap#inspect only displays size in output diff --git a/spec/tags/core/objectspace/weakkeymap/key_tags.txt b/spec/tags/core/objectspace/weakkeymap/key_tags.txt deleted file mode 100644 index 3b4e1808929..00000000000 --- a/spec/tags/core/objectspace/weakkeymap/key_tags.txt +++ /dev/null @@ -1,3 +0,0 @@ -fails:ObjectSpace::WeakKeyMap#key? recognizes keys in use -fails:ObjectSpace::WeakKeyMap#key? matches using equality semantics -fails:ObjectSpace::WeakKeyMap#key? reports true if the pair exists and the value is nil diff --git a/spec/truffle/methods/ObjectSpace::WeakKeyMap.txt b/spec/truffle/methods/ObjectSpace::WeakKeyMap.txt new file mode 100644 index 00000000000..e85237c8e2b --- /dev/null +++ b/spec/truffle/methods/ObjectSpace::WeakKeyMap.txt @@ -0,0 +1,7 @@ +[] +[]= +clear +delete +getkey +inspect +key? diff --git a/spec/truffle/methods/ObjectSpace::WeakMap.txt b/spec/truffle/methods/ObjectSpace::WeakMap.txt new file mode 100644 index 00000000000..ff6cf97c92a --- /dev/null +++ b/spec/truffle/methods/ObjectSpace::WeakMap.txt @@ -0,0 +1,15 @@ +[] +[]= +delete +each +each_key +each_pair +each_value +include? +inspect +key? +keys +length +member? +size +values diff --git a/spec/truffle/methods_spec.rb b/spec/truffle/methods_spec.rb index 9437b37d95e..7ff07b4406d 100644 --- a/spec/truffle/methods_spec.rb +++ b/spec/truffle/methods_spec.rb @@ -24,7 +24,8 @@ Array BasicObject Binding Class Complex Complex Data Dir ENV.singleton_class Encoding Enumerable Enumerator Enumerator::Lazy Exception FalseClass Fiber File FileTest Float GC GC.singleton_class Hash IO Integer Kernel Marshal MatchData Math Method - Module Mutex NilClass Numeric Object ObjectSpace Proc Process Process.singleton_class Queue Random + Module Mutex NilClass Numeric Object ObjectSpace ObjectSpace::WeakKeyMap ObjectSpace::WeakMap + Proc Process Process.singleton_class Queue Random Random::Formatter Random.singleton_class Range Rational Regexp Signal SizedQueue String Struct Symbol SystemExit Thread TracePoint TrueClass UnboundMethod Warning diff --git a/src/main/java/org/truffleruby/RubyLanguage.java b/src/main/java/org/truffleruby/RubyLanguage.java index e0c696ea41c..80fb9a862fa 100644 --- a/src/main/java/org/truffleruby/RubyLanguage.java +++ b/src/main/java/org/truffleruby/RubyLanguage.java @@ -74,6 +74,7 @@ import org.truffleruby.core.mutex.RubyConditionVariable; import org.truffleruby.core.mutex.RubyMutex; import org.truffleruby.core.objectspace.ObjectSpaceManager; +import org.truffleruby.core.objectspace.RubyWeakKeyMap; import org.truffleruby.core.objectspace.RubyWeakMap; import org.truffleruby.core.proc.RubyProc; import org.truffleruby.core.queue.RubyQueue; @@ -332,6 +333,7 @@ private RubyThread getOrCreateForeignThread(RubyContext context, Thread thread) public final Shape truffleFFIPointerShape = createShape(RubyPointer.class); public final Shape unboundMethodShape = createShape(RubyUnboundMethod.class); public final Shape weakMapShape = createShape(RubyWeakMap.class); + public final Shape weakKeyMapShape = createShape(RubyWeakKeyMap.class); public final Shape classVariableShape = Shape .newBuilder() diff --git a/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java b/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java index 3339d52a604..b3f040e0a20 100644 --- a/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java +++ b/src/main/java/org/truffleruby/builtins/BuiltinsClasses.java @@ -76,6 +76,8 @@ import org.truffleruby.core.method.UnboundMethodNodesFactory; import org.truffleruby.core.module.ModuleNodesBuiltins; import org.truffleruby.core.module.ModuleNodesFactory; +import org.truffleruby.core.objectspace.WeakKeyMapNodesBuiltins; +import org.truffleruby.core.objectspace.WeakKeyMapNodesFactory; import org.truffleruby.core.refinement.RefinementNodesBuiltins; import org.truffleruby.core.refinement.RefinementNodesFactory; import org.truffleruby.core.monitor.TruffleMonitorNodesBuiltins; @@ -254,6 +256,7 @@ public static void setupBuiltinsLazy(CoreMethodNodeManager coreManager) { TypeNodesBuiltins.setup(coreManager); UnboundMethodNodesBuiltins.setup(coreManager); VMPrimitiveNodesBuiltins.setup(coreManager); + WeakKeyMapNodesBuiltins.setup(coreManager); WeakMapNodesBuiltins.setup(coreManager); WeakRefNodesBuiltins.setup(coreManager); } @@ -336,6 +339,7 @@ public static void setupBuiltinsLazyPrimitives(PrimitiveManager primitiveManager TypeNodesBuiltins.setupPrimitives(primitiveManager); UnboundMethodNodesBuiltins.setupPrimitives(primitiveManager); VMPrimitiveNodesBuiltins.setupPrimitives(primitiveManager); + WeakKeyMapNodesBuiltins.setupPrimitives(primitiveManager); WeakMapNodesBuiltins.setupPrimitives(primitiveManager); WeakRefNodesBuiltins.setupPrimitives(primitiveManager); } @@ -419,6 +423,7 @@ public static List>> getCoreN TypeNodesFactory.getFactories(), UnboundMethodNodesFactory.getFactories(), VMPrimitiveNodesFactory.getFactories(), + WeakKeyMapNodesFactory.getFactories(), WeakMapNodesFactory.getFactories(), WeakRefNodesFactory.getFactories()); } diff --git a/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java b/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java index dbaee24e294..dfc53d71220 100644 --- a/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java +++ b/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java @@ -18,24 +18,34 @@ import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; -/** A concurrent thread-safe map with weak keys. Keys cannot be null. This map currently assumes keys do not depend on - * Hashing and so there is no need for ReHashable. So far this is only used for keys which are compared by identity. */ +/** A concurrent thread-safe map with weak keys. Keys cannot be null. This map currently assumes keys never need to be + * re-hashed with ReHashable, which means no Ruby ObjectSpace::WeakKeyMap instance should be part of the pre-initialized + * context. ConcurrentWeakKeysMap and ConcurrentWeakSet with keys whose hashCode() does not depend on the Ruby random + * seed are fine as part of the pre-initialized context. */ public class ConcurrentWeakKeysMap { protected final ConcurrentHashMap, Value> map = new ConcurrentHashMap<>(); private final ReferenceQueue referenceQueue = new ReferenceQueue<>(); + @TruffleBoundary public ConcurrentWeakKeysMap() { } @TruffleBoundary - public Value get(Key key) { + public void clear() { + map.clear(); + } + + @TruffleBoundary + public boolean containsKey(Key key) { removeStaleEntries(); - return map.get(new WeakKeyReference<>(key)); + return map.containsKey(new WeakKeyReference<>(key)); } - public boolean contains(Key key) { - return get(key) != null; + @TruffleBoundary + public Value get(Key key) { + removeStaleEntries(); + return map.get(new WeakKeyReference<>(key)); } /** Sets the value in the cache, always returns the old value. */ @@ -46,12 +56,28 @@ public Value put(Key key, Value value) { return map.put(ref, value); } + @TruffleBoundary + public int size() { + removeStaleEntries(); + int size = 0; + + // Filter out null entries. + for (var e : map.keySet()) { + final Key key = e.get(); + if (key != null) { + ++size; + } + } + + return size; + } + @TruffleBoundary public Collection keys() { removeStaleEntries(); final Collection keys = new ArrayList<>(map.size()); - // Filter out keys for null values. + // Filter out null entries. for (var e : map.keySet()) { final Key key = e.get(); if (key != null) { @@ -62,6 +88,12 @@ public Collection keys() { return keys; } + @TruffleBoundary + public Value remove(Key key) { + removeStaleEntries(); + return map.remove(new WeakKeyReference<>(key)); + } + /** Attempts to remove map entries whose values have been made unreachable by the GC. *

* This relies on the underlying {@link WeakReference} instance being enqueued to the {@link #referenceQueue} queue. @@ -96,8 +128,7 @@ public boolean equals(Object other) { if (this == other) { return true; } - if (other instanceof WeakKeyReference) { - WeakKeyReference ref = (WeakKeyReference) other; + if (other instanceof WeakKeyReference ref) { Key key = get(); Object otherKey = ref.get(); return key != null && otherKey != null && key.equals(otherKey); diff --git a/src/main/java/org/truffleruby/collections/ConcurrentWeakSet.java b/src/main/java/org/truffleruby/collections/ConcurrentWeakSet.java index b4972efb473..efbbfc3c1ca 100644 --- a/src/main/java/org/truffleruby/collections/ConcurrentWeakSet.java +++ b/src/main/java/org/truffleruby/collections/ConcurrentWeakSet.java @@ -25,6 +25,11 @@ public boolean add(E element) { return put(element, Boolean.TRUE) == null; } + @TruffleBoundary + public boolean contains(E element) { + return containsKey(element); + } + @TruffleBoundary public Object[] toArray() { return keys().toArray(); diff --git a/src/main/java/org/truffleruby/collections/WeakValueCache.java b/src/main/java/org/truffleruby/collections/WeakValueCache.java index 68c9c63f043..c2417343534 100644 --- a/src/main/java/org/truffleruby/collections/WeakValueCache.java +++ b/src/main/java/org/truffleruby/collections/WeakValueCache.java @@ -106,8 +106,8 @@ public Value put(Key key, Value value) { @TruffleBoundary public int size() { - int size = 0; removeStaleEntries(); + int size = 0; // Filter out null entries. for (KeyedReference ref : map.values()) { @@ -132,7 +132,7 @@ public Collection keys() { removeStaleEntries(); final Collection keys = new ArrayList<>(map.size()); - // Filter out keys for null values. + // Filter out null entries. for (Entry> e : map.entrySet()) { final Value value = e.getValue().get(); if (value != null) { @@ -148,7 +148,7 @@ public Collection values() { removeStaleEntries(); final Collection values = new ArrayList<>(map.size()); - // Filter out null values. + // Filter out null entries. for (WeakReference reference : map.values()) { final Value value = reference.get(); if (value != null) { @@ -164,7 +164,7 @@ public Collection> entries() { removeStaleEntries(); final Collection> entries = new ArrayList<>(map.size()); - // Filter out null values. + // Filter out null entries. for (Entry> e : map.entrySet()) { final Value value = e.getValue().get(); if (value != null) { diff --git a/src/main/java/org/truffleruby/core/CoreLibrary.java b/src/main/java/org/truffleruby/core/CoreLibrary.java index 21157fa4c93..18769478795 100644 --- a/src/main/java/org/truffleruby/core/CoreLibrary.java +++ b/src/main/java/org/truffleruby/core/CoreLibrary.java @@ -222,6 +222,7 @@ public final class CoreLibrary { public final RubyClass digestClass; public final RubyClass structClass; public final RubyClass weakMapClass; + public final RubyClass weakKeyMapClass; public final RubyArray argv; public final RubyBasicObject mainObject; @@ -445,6 +446,7 @@ public CoreLibrary(RubyContext context, RubyLanguage language) { objectSpaceModule = defineModule("ObjectSpace"); weakMapClass = defineClass(objectSpaceModule, objectClass, "WeakMap"); + weakKeyMapClass = defineClass(objectSpaceModule, objectClass, "WeakKeyMap"); // The rest @@ -1049,6 +1051,7 @@ public boolean isSplittingEnabled() { "/core/unbound_method.rb", "/core/truffle/warning_operations.rb", "/core/warning.rb", + "/core/weakkeymap.rb", "/core/weakmap.rb", "/core/tracepoint.rb", "/core/truffle/interop.rb", diff --git a/src/main/java/org/truffleruby/core/hash/CompareByRubyHashEqlWrapper.java b/src/main/java/org/truffleruby/core/hash/CompareByRubyHashEqlWrapper.java new file mode 100644 index 00000000000..4f50212987c --- /dev/null +++ b/src/main/java/org/truffleruby/core/hash/CompareByRubyHashEqlWrapper.java @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2013, 2025 Oracle and/or its affiliates. All rights reserved. This + * code is released under a tri EPL/GPL/LGPL license. You can use it, + * redistribute it and/or modify it under the terms of the: + * + * Eclipse Public License version 2.0, or + * GNU General Public License version 2, or + * GNU Lesser General Public License version 2.1. + */ +package org.truffleruby.core.hash; + +import org.truffleruby.core.hash.HashingNodes.ToHashByHashCode; +import org.truffleruby.core.kernel.KernelNodes.SameOrEqlNode; + +/** Wraps a key so that it will be hashed and compared according to Ruby Hash key semantics, using #hash and #eql?. */ +public class CompareByRubyHashEqlWrapper { + + private final Object key; + private final int hashCode; + + /** @param hashCode the result of {@link ToHashByHashCode} on the key */ + public CompareByRubyHashEqlWrapper(Object key, int hashCode) { + this.key = key; + this.hashCode = hashCode; + } + + @Override + public int hashCode() { + return hashCode; + } + + @Override + public boolean equals(Object obj) { + // We use an uncached SameOrEqlNode here since we are not in PE-able code + return obj instanceof CompareByRubyHashEqlWrapper other && hashCode == other.hashCode && + SameOrEqlNode.executeUncached(key, other.key); + } + + public static final class RecordingCompareByRubyHashEqlWrapper extends CompareByRubyHashEqlWrapper { + + public Object keyInMap; + + public RecordingCompareByRubyHashEqlWrapper(Object key, int hashCode) { + super(key, hashCode); + } + + @Override + public int hashCode() { + return super.hashCode(); + } + + @Override + public boolean equals(Object obj) { + if (super.equals(obj)) { + this.keyInMap = ((CompareByRubyHashEqlWrapper) obj).key; + return true; + } else { + return false; + } + } + } +} diff --git a/src/main/java/org/truffleruby/core/hash/CompareByRubyIdentityWrapper.java b/src/main/java/org/truffleruby/core/hash/CompareByRubyIdentityWrapper.java index b7baab53ddc..bae8ee7e901 100644 --- a/src/main/java/org/truffleruby/core/hash/CompareByRubyIdentityWrapper.java +++ b/src/main/java/org/truffleruby/core/hash/CompareByRubyIdentityWrapper.java @@ -12,25 +12,26 @@ import org.truffleruby.core.basicobject.ReferenceEqualNode; import org.truffleruby.core.hash.HashingNodes.ToHashByIdentity; -/** Wraps a value so that it will be compared and hashed according to Ruby identity semantics. These semantics differ from +/** Wraps a key so that it will be hashed and compared according to Ruby identity semantics. These semantics differ from * Java semantics notably for primitives (e.g. all the NaN are different according to Ruby, but Java compares them * equal). */ public final class CompareByRubyIdentityWrapper { - public final Object value; + public final Object key; - public CompareByRubyIdentityWrapper(Object value) { - this.value = value; + public CompareByRubyIdentityWrapper(Object key) { + this.key = key; } @Override public int hashCode() { - return ToHashByIdentity.getUncached().execute(value); + return ToHashByIdentity.getUncached().execute(key); } @Override public boolean equals(Object obj) { + // We use an uncached ReferenceEqualNode here since we are not in PE-able code return obj instanceof CompareByRubyIdentityWrapper && - ReferenceEqualNode.executeUncached(value, ((CompareByRubyIdentityWrapper) obj).value); + ReferenceEqualNode.executeUncached(key, ((CompareByRubyIdentityWrapper) obj).key); } } diff --git a/src/main/java/org/truffleruby/core/hash/HashingNodes.java b/src/main/java/org/truffleruby/core/hash/HashingNodes.java index a82040b2097..7f5c0cc97c7 100644 --- a/src/main/java/org/truffleruby/core/hash/HashingNodes.java +++ b/src/main/java/org/truffleruby/core/hash/HashingNodes.java @@ -67,6 +67,10 @@ public static ToHashByHashCode create() { return HashingNodesFactory.ToHashByHashCodeNodeGen.create(); } + public static int executeUncached(Object key) { + return HashingNodesFactory.ToHashByHashCodeNodeGen.getUncached().execute(null, key); + } + public abstract int execute(Node node, Object key); public final int executeCached(Object key) { diff --git a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java index 2f9c259f4dd..4cc1452dd43 100644 --- a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java +++ b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java @@ -1750,17 +1750,6 @@ RubyArray globalVariables() { } - @Primitive(name = "kernel_to_hex") - public abstract static class KernelToHexStringNode extends PrimitiveArrayArgumentsNode { - - @Specialization - String toHexString(Object value, - @Cached ToHexStringNode toHexStringNode) { - return toHexStringNode.execute(this, value); - } - } - - @GenerateUncached @GenerateInline @GenerateCached(false) diff --git a/src/main/java/org/truffleruby/core/objectspace/RubyWeakKeyMap.java b/src/main/java/org/truffleruby/core/objectspace/RubyWeakKeyMap.java new file mode 100644 index 00000000000..026383f2b5a --- /dev/null +++ b/src/main/java/org/truffleruby/core/objectspace/RubyWeakKeyMap.java @@ -0,0 +1,24 @@ +/* + * Copyright (c) 2025 Oracle and/or its affiliates. All rights reserved. This + * code is released under a tri EPL/GPL/LGPL license. You can use it, + * redistribute it and/or modify it under the terms of the: + * + * Eclipse Public License version 2.0, or + * GNU General Public License version 2, or + * GNU Lesser General Public License version 2.1. + */ +package org.truffleruby.core.objectspace; + +import com.oracle.truffle.api.object.Shape; +import org.truffleruby.core.klass.RubyClass; +import org.truffleruby.language.RubyDynamicObject; + +public final class RubyWeakKeyMap extends RubyDynamicObject { + + final WeakKeyMapStorage storage = new WeakKeyMapStorage(); + + public RubyWeakKeyMap(RubyClass rubyClass, Shape shape) { + super(rubyClass, shape); + } + +} diff --git a/src/main/java/org/truffleruby/core/objectspace/RubyWeakMap.java b/src/main/java/org/truffleruby/core/objectspace/RubyWeakMap.java index 44a8c330c75..2a6dfa7430c 100644 --- a/src/main/java/org/truffleruby/core/objectspace/RubyWeakMap.java +++ b/src/main/java/org/truffleruby/core/objectspace/RubyWeakMap.java @@ -15,11 +15,10 @@ public final class RubyWeakMap extends RubyDynamicObject { - final WeakMapStorage storage; + final WeakMapStorage storage = new WeakMapStorage(); - public RubyWeakMap(RubyClass rubyClass, Shape shape, WeakMapStorage storage) { + public RubyWeakMap(RubyClass rubyClass, Shape shape) { super(rubyClass, shape); - this.storage = storage; } } diff --git a/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java b/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java new file mode 100644 index 00000000000..fed9bebea07 --- /dev/null +++ b/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java @@ -0,0 +1,148 @@ +/* + * Copyright (c) 2025 Oracle and/or its affiliates. All rights reserved. This + * code is released under a tri EPL/GPL/LGPL license. You can use it, + * redistribute it and/or modify it under the terms of the: + * + * Eclipse Public License version 2.0, or + * GNU General Public License version 2, or + * GNU Lesser General Public License version 2.1. + */ +package org.truffleruby.core.objectspace; + +import com.oracle.truffle.api.dsl.Cached; +import com.oracle.truffle.api.profiles.InlinedConditionProfile; +import org.truffleruby.annotations.CoreMethod; +import org.truffleruby.annotations.CoreModule; +import org.truffleruby.annotations.Primitive; +import org.truffleruby.builtins.CoreMethodArrayArgumentsNode; +import org.truffleruby.builtins.PrimitiveArrayArgumentsNode; +import org.truffleruby.core.hash.CompareByRubyHashEqlWrapper; +import org.truffleruby.core.hash.CompareByRubyHashEqlWrapper.RecordingCompareByRubyHashEqlWrapper; +import org.truffleruby.core.hash.HashingNodes.ToHashByHashCode; +import org.truffleruby.core.klass.RubyClass; +import org.truffleruby.core.proc.RubyProc; +import org.truffleruby.language.Nil; +import org.truffleruby.annotations.Visibility; +import com.oracle.truffle.api.dsl.Specialization; +import org.truffleruby.language.objects.AllocationTracing; +import org.truffleruby.language.yield.CallBlockNode; + +import java.util.Objects; + +@CoreModule(value = "ObjectSpace::WeakKeyMap", isClass = true) +public abstract class WeakKeyMapNodes { + + @CoreMethod(names = { "__allocate__", "__layout_allocate__" }, constructor = true, visibility = Visibility.PRIVATE) + public abstract static class AllocateNode extends CoreMethodArrayArgumentsNode { + + @Specialization + RubyWeakKeyMap allocate(RubyClass rubyClass) { + final RubyWeakKeyMap weakKeyMap = new RubyWeakKeyMap(rubyClass, getLanguage().weakKeyMapShape); + AllocationTracing.trace(weakKeyMap, this); + return weakKeyMap; + } + } + + @Primitive(name = "weakkeymap_size") + public abstract static class SizeNode extends PrimitiveArrayArgumentsNode { + + @Specialization + int size(RubyWeakKeyMap map) { + return map.storage.size(); + } + } + + @CoreMethod(names = "[]", required = 1) + public abstract static class GetIndexNode extends CoreMethodArrayArgumentsNode { + + @Specialization + Object get(RubyWeakKeyMap map, Object key, + @Cached ToHashByHashCode hashCodeNode) { + int hashCode = hashCodeNode.execute(this, key); + Object value = map.storage.get(new CompareByRubyHashEqlWrapper(key, hashCode)); + return value == null ? nil : value; + } + } + + @CoreMethod(names = "getkey", required = 1) + public abstract static class GetKeyNode extends CoreMethodArrayArgumentsNode { + + @Specialization + Object getKey(RubyWeakKeyMap map, Object key, + @Cached ToHashByHashCode hashCodeNode) { + int hashCode = hashCodeNode.execute(this, key); + var recordingWrapper = new RecordingCompareByRubyHashEqlWrapper(key, hashCode); + if (map.storage.containsKey(recordingWrapper)) { + return Objects.requireNonNull(recordingWrapper.keyInMap); + } else { + return nil; + } + } + } + + @CoreMethod(names = "key?", required = 1) + public abstract static class MemberNode extends CoreMethodArrayArgumentsNode { + + @Specialization + boolean containsKey(RubyWeakKeyMap map, Object key, + @Cached ToHashByHashCode hashCodeNode) { + int hashCode = hashCodeNode.execute(this, key); + return map.storage.containsKey(new CompareByRubyHashEqlWrapper(key, hashCode)); + } + } + + @CoreMethod(names = "[]=", required = 2) + public abstract static class SetIndexNode extends CoreMethodArrayArgumentsNode { + + @Specialization + Object set(RubyWeakKeyMap map, Object key, Object value, + @Cached ToHashByHashCode hashCodeNode) { + int hashCode = hashCodeNode.execute(this, key); + map.storage.put(new CompareByRubyHashEqlWrapper(key, hashCode), value); + return value; + } + } + + @CoreMethod(names = "clear") + public abstract static class ClearNode extends CoreMethodArrayArgumentsNode { + + @Specialization + RubyWeakKeyMap clear(RubyWeakKeyMap map) { + map.storage.clear(); + return map; + } + } + + @CoreMethod(names = "delete", required = 1, needsBlock = true) + public abstract static class DeleteNode extends CoreMethodArrayArgumentsNode { + + @Specialization + Object delete(RubyWeakKeyMap map, Object key, RubyProc block, + @Cached InlinedConditionProfile isContainedProfile, + @Cached CallBlockNode yieldNode, + @Cached ToHashByHashCode hashCodeNode) { + int hashCode = hashCodeNode.execute(this, key); + Object value = map.storage.remove(new CompareByRubyHashEqlWrapper(key, hashCode)); + + if (isContainedProfile.profile(this, value != null)) { + return value; + } else { + return yieldNode.yield(this, block, key); + } + } + + @Specialization + Object delete(RubyWeakKeyMap map, Object key, Nil block, + @Cached InlinedConditionProfile isContainedProfile, + @Cached ToHashByHashCode hashCodeNode) { + int hashCode = hashCodeNode.execute(this, key); + Object value = map.storage.remove(new CompareByRubyHashEqlWrapper(key, hashCode)); + + if (isContainedProfile.profile(this, value != null)) { + return value; + } else { + return nil; + } + } + } +} diff --git a/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapStorage.java b/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapStorage.java new file mode 100644 index 00000000000..d6fe0682200 --- /dev/null +++ b/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapStorage.java @@ -0,0 +1,17 @@ +/* + * Copyright (c) 2025 Oracle and/or its affiliates. All rights reserved. This + * code is released under a tri EPL/GPL/LGPL license. You can use it, + * redistribute it and/or modify it under the terms of the: + * + * Eclipse Public License version 2.0, or + * GNU General Public License version 2, or + * GNU Lesser General Public License version 2.1. + */ +package org.truffleruby.core.objectspace; + +import org.truffleruby.collections.ConcurrentWeakKeysMap; +import org.truffleruby.core.hash.CompareByRubyHashEqlWrapper; + +// An alias of ConcurrentWeakKeysMap to keep things short +public final class WeakKeyMapStorage extends ConcurrentWeakKeysMap { +} diff --git a/src/main/java/org/truffleruby/core/objectspace/WeakMapNodes.java b/src/main/java/org/truffleruby/core/objectspace/WeakMapNodes.java index e5cc687de78..da263bc275e 100644 --- a/src/main/java/org/truffleruby/core/objectspace/WeakMapNodes.java +++ b/src/main/java/org/truffleruby/core/objectspace/WeakMapNodes.java @@ -14,9 +14,7 @@ import org.truffleruby.RubyContext; import org.truffleruby.annotations.CoreMethod; import org.truffleruby.annotations.CoreModule; -import org.truffleruby.annotations.Primitive; import org.truffleruby.builtins.CoreMethodArrayArgumentsNode; -import org.truffleruby.builtins.PrimitiveArrayArgumentsNode; import org.truffleruby.collections.SimpleEntry; import org.truffleruby.core.array.RubyArray; import org.truffleruby.core.hash.CompareByRubyIdentityWrapper; @@ -44,7 +42,7 @@ public abstract static class AllocateNode extends CoreMethodArrayArgumentsNode { @Specialization RubyWeakMap allocate(RubyClass rubyClass) { - final RubyWeakMap weakMap = new RubyWeakMap(rubyClass, getLanguage().weakMapShape, new WeakMapStorage()); + final RubyWeakMap weakMap = new RubyWeakMap(rubyClass, getLanguage().weakMapShape); AllocationTracing.trace(weakMap, this); return weakMap; } @@ -78,8 +76,8 @@ Object get(RubyWeakMap map, Object key) { } } - @Primitive(name = "weakmap_aset") - public abstract static class SetIndexNode extends PrimitiveArrayArgumentsNode { + @CoreMethod(names = "[]=", required = 2) + public abstract static class SetIndexNode extends CoreMethodArrayArgumentsNode { @Specialization Object set(RubyWeakMap map, Object key, Object value) { @@ -88,7 +86,7 @@ Object set(RubyWeakMap map, Object key, Object value) { } } - @CoreMethod(names = { "keys" }) + @CoreMethod(names = "keys") public abstract static class KeysNode extends CoreMethodArrayArgumentsNode { @Specialization @@ -97,7 +95,7 @@ RubyArray getKeys(RubyWeakMap map) { } } - @CoreMethod(names = { "values" }) + @CoreMethod(names = "values") public abstract static class ValuesNode extends CoreMethodArrayArgumentsNode { @Specialization @@ -135,7 +133,7 @@ Object delete(RubyWeakMap map, Object key, Nil block, } } - @CoreMethod(names = { "each_key" }, needsBlock = true) + @CoreMethod(names = "each_key", needsBlock = true) public abstract static class EachKeyNode extends CoreMethodArrayArgumentsNode { @Specialization @@ -153,7 +151,7 @@ RubyWeakMap eachKey(RubyWeakMap map, RubyProc block, } } - @CoreMethod(names = { "each_value" }, needsBlock = true) + @CoreMethod(names = "each_value", needsBlock = true) public abstract static class EachValueNode extends CoreMethodArrayArgumentsNode { @Specialization @@ -197,7 +195,7 @@ private static Object[] keys(WeakMapStorage storage) { final Object[] keys = new Object[keyWrappers.size()]; int i = 0; for (CompareByRubyIdentityWrapper keyWrapper : keyWrappers) { - keys[i++] = keyWrapper.value; + keys[i++] = keyWrapper.key; } return keys; } @@ -213,7 +211,7 @@ private static Object[] values(WeakMapStorage storage) { final SimpleEntry[] entries = new SimpleEntry[wrappedEntries.size()]; int i = 0; for (SimpleEntry wrappedEntry : wrappedEntries) { - entries[i++] = new SimpleEntry<>(wrappedEntry.getKey().value, wrappedEntry.getValue()); + entries[i++] = new SimpleEntry<>(wrappedEntry.getKey().key, wrappedEntry.getValue()); } return entries; } diff --git a/src/main/java/org/truffleruby/core/objectspace/WeakMapStorage.java b/src/main/java/org/truffleruby/core/objectspace/WeakMapStorage.java index a7cc5351cdd..a71acf7e455 100644 --- a/src/main/java/org/truffleruby/core/objectspace/WeakMapStorage.java +++ b/src/main/java/org/truffleruby/core/objectspace/WeakMapStorage.java @@ -12,5 +12,6 @@ import org.truffleruby.collections.WeakValueCache; import org.truffleruby.core.hash.CompareByRubyIdentityWrapper; +// An alias of WeakValueCache to keep things short public final class WeakMapStorage extends WeakValueCache { } diff --git a/src/main/ruby/truffleruby/core/weakkeymap.rb b/src/main/ruby/truffleruby/core/weakkeymap.rb new file mode 100644 index 00000000000..25352b00b59 --- /dev/null +++ b/src/main/ruby/truffleruby/core/weakkeymap.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +# Copyright (c) 2025 Oracle and/or its affiliates. All rights reserved. This +# code is released under a tri EPL/GPL/LGPL license. You can use it, +# redistribute it and/or modify it under the terms of the: +# +# Eclipse Public License version 2.0, or +# GNU General Public License version 2, or +# GNU Lesser General Public License version 2.1. + +module ObjectSpace + # WeakKeyMap uses #hash and #eql? to compare keys, like Hash + class WeakKeyMap + def inspect + "#{super[0...-1]} size=#{Primitive.weakkeymap_size(self)}>" + end + end +end diff --git a/src/main/ruby/truffleruby/core/weakmap.rb b/src/main/ruby/truffleruby/core/weakmap.rb index 21699b3cd9d..3ba887bab33 100644 --- a/src/main/ruby/truffleruby/core/weakmap.rb +++ b/src/main/ruby/truffleruby/core/weakmap.rb @@ -13,12 +13,8 @@ module ObjectSpace class WeakMap include Enumerable - def []=(key, value) - Primitive.weakmap_aset(self, key, value) - end - def inspect - str = "# 0 entries.each do |k, v| From f8390c651998e17e1f52435a42e7e6b2f7bf8a3a Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Wed, 29 Jan 2025 20:44:11 +0200 Subject: [PATCH 3/6] Fix #[]= and raise ArgumentError when key is not garbage collectable --- .../objectspace/weakkeymap/delete_spec.rb | 11 ++++ .../weakkeymap/element_reference_spec.rb | 19 ++++--- .../weakkeymap/element_set_spec.rb | 52 +++++++++---------- .../objectspace/weakkeymap/getkey_spec.rb | 11 ++++ .../core/objectspace/weakkeymap/key_spec.rb | 11 ++++ .../weakkeymap/element_reference_tags.txt | 1 - .../weakkeymap/element_set_tags.txt | 5 -- .../core/objectspace/WeakKeyMapNodes.java | 17 +++++- 8 files changed, 84 insertions(+), 43 deletions(-) delete mode 100644 spec/tags/core/objectspace/weakkeymap/element_reference_tags.txt delete mode 100644 spec/tags/core/objectspace/weakkeymap/element_set_tags.txt diff --git a/spec/ruby/core/objectspace/weakkeymap/delete_spec.rb b/spec/ruby/core/objectspace/weakkeymap/delete_spec.rb index 6e534b8ea8b..0086cd2a406 100644 --- a/spec/ruby/core/objectspace/weakkeymap/delete_spec.rb +++ b/spec/ruby/core/objectspace/weakkeymap/delete_spec.rb @@ -36,5 +36,16 @@ m = ObjectSpace::WeakMap.new m.delete(Object.new).should == nil end + + it "returns nil when a key cannot be garbage collected" do + map = ObjectSpace::WeakKeyMap.new + + map.delete(1).should == nil + map.delete(1.0).should == nil + map.delete(:a).should == nil + map.delete(true).should == nil + map.delete(false).should == nil + map.delete(nil).should == nil + end end end diff --git a/spec/ruby/core/objectspace/weakkeymap/element_reference_spec.rb b/spec/ruby/core/objectspace/weakkeymap/element_reference_spec.rb index c91375f9f0d..365f7f0f394 100644 --- a/spec/ruby/core/objectspace/weakkeymap/element_reference_spec.rb +++ b/spec/ruby/core/objectspace/weakkeymap/element_reference_spec.rb @@ -88,16 +88,15 @@ map[key].should == 42 end - it "raises ArgumentError when numbers or Symbols are used as keys, that cannot be garbage collected" do - -> { - map = ObjectSpace::WeakKeyMap.new - map[1.0] = "x" - }.should raise_error(ArgumentError, "WeakKeyMap must be garbage collectable") - - -> { - map = ObjectSpace::WeakKeyMap.new - map[:a] = "x" - }.should raise_error(ArgumentError, "WeakKeyMap must be garbage collectable") + it "returns nil and does not raise error when a key cannot be garbage collected" do + map = ObjectSpace::WeakKeyMap.new + + map[1].should == nil + map[1.0].should == nil + map[:a].should == nil + map[true].should == nil + map[false].should == nil + map[nil].should == nil end end end diff --git a/spec/ruby/core/objectspace/weakkeymap/element_set_spec.rb b/spec/ruby/core/objectspace/weakkeymap/element_set_spec.rb index c427e01ca5a..f08ae101b03 100644 --- a/spec/ruby/core/objectspace/weakkeymap/element_set_spec.rb +++ b/spec/ruby/core/objectspace/weakkeymap/element_set_spec.rb @@ -8,10 +8,6 @@ def should_accept(map, key, value) map[key].should == value end - def should_not_accept(map, key, value) - -> { map[key] = value }.should raise_error(ArgumentError) - end - it "is correct" do map = ObjectSpace::WeakKeyMap.new key1, key2 = %w[a b].map(&:upcase) @@ -40,32 +36,36 @@ def should_not_accept(map, key, value) should_accept(map, y, x) end - it "rejects symbols as keys" do - map = ObjectSpace::WeakKeyMap.new - should_not_accept(map, :foo, true) - should_not_accept(map, rand.to_s.to_sym, true) - end + context "a key cannot be garbage collected" do + it "raises ArgumentError when Integer is used as a key" do + map = ObjectSpace::WeakKeyMap.new + -> { map[1] = "x" }.should raise_error(ArgumentError, "WeakKeyMap must be garbage collectable") + end - it "rejects integers as keys" do - map = ObjectSpace::WeakKeyMap.new - should_not_accept(map, 42, true) - should_not_accept(map, 2 ** 68, true) - end + it "raises ArgumentError when Float is used as a key" do + map = ObjectSpace::WeakKeyMap.new + -> { map[1.0] = "x" }.should raise_error(ArgumentError, "WeakKeyMap must be garbage collectable") + end - it "rejects floats as keys" do - map = ObjectSpace::WeakKeyMap.new - should_not_accept(map, 4.2, true) - end + it "raises ArgumentError when Symbol is used as a key" do + map = ObjectSpace::WeakKeyMap.new + -> { map[:a] = "x" }.should raise_error(ArgumentError, "WeakKeyMap must be garbage collectable") + end - it "rejects booleans as keys" do - map = ObjectSpace::WeakKeyMap.new - should_not_accept(map, true, true) - should_not_accept(map, false, true) - end + it "raises ArgumentError when true is used as a key" do + map = ObjectSpace::WeakKeyMap.new + -> { map[true] = "x" }.should raise_error(ArgumentError, "WeakKeyMap must be garbage collectable") + end - it "rejects nil as keys" do - map = ObjectSpace::WeakKeyMap.new - should_not_accept(map, nil, true) + it "raises ArgumentError when false is used as a key" do + map = ObjectSpace::WeakKeyMap.new + -> { map[false] = "x" }.should raise_error(ArgumentError, "WeakKeyMap must be garbage collectable") + end + + it "raises ArgumentError when nil is used as a key" do + map = ObjectSpace::WeakKeyMap.new + -> { map[nil] = "x" }.should raise_error(ArgumentError, "WeakKeyMap must be garbage collectable") + end end end end diff --git a/spec/ruby/core/objectspace/weakkeymap/getkey_spec.rb b/spec/ruby/core/objectspace/weakkeymap/getkey_spec.rb index 3af0186f278..c30a29a18b6 100644 --- a/spec/ruby/core/objectspace/weakkeymap/getkey_spec.rb +++ b/spec/ruby/core/objectspace/weakkeymap/getkey_spec.rb @@ -10,5 +10,16 @@ map.getkey(key2).should equal(key1) map.getkey("X").should == nil end + + it "returns nil when a key cannot be garbage collected" do + map = ObjectSpace::WeakKeyMap.new + + map.getkey(1).should == nil + map.getkey(1.0).should == nil + map.getkey(:a).should == nil + map.getkey(true).should == nil + map.getkey(false).should == nil + map.getkey(nil).should == nil + end end end diff --git a/spec/ruby/core/objectspace/weakkeymap/key_spec.rb b/spec/ruby/core/objectspace/weakkeymap/key_spec.rb index 2af9c2b8e79..a9a2e12432c 100644 --- a/spec/ruby/core/objectspace/weakkeymap/key_spec.rb +++ b/spec/ruby/core/objectspace/weakkeymap/key_spec.rb @@ -29,5 +29,16 @@ map[key] = nil map.key?(key).should == true end + + it "returns false when a key cannot be garbage collected" do + map = ObjectSpace::WeakKeyMap.new + + map.key?(1).should == false + map.key?(1.0).should == false + map.key?(:a).should == false + map.key?(true).should == false + map.key?(false).should == false + map.key?(nil).should == false + end end end diff --git a/spec/tags/core/objectspace/weakkeymap/element_reference_tags.txt b/spec/tags/core/objectspace/weakkeymap/element_reference_tags.txt deleted file mode 100644 index 5b41b6fdcb1..00000000000 --- a/spec/tags/core/objectspace/weakkeymap/element_reference_tags.txt +++ /dev/null @@ -1 +0,0 @@ -fails:ObjectSpace::WeakKeyMap#[] raises ArgumentError when numbers or Symbols are used as keys, that cannot be garbage collected diff --git a/spec/tags/core/objectspace/weakkeymap/element_set_tags.txt b/spec/tags/core/objectspace/weakkeymap/element_set_tags.txt deleted file mode 100644 index c86bda7de68..00000000000 --- a/spec/tags/core/objectspace/weakkeymap/element_set_tags.txt +++ /dev/null @@ -1,5 +0,0 @@ -fails:ObjectSpace::WeakKeyMap#[]= rejects symbols as keys -fails:ObjectSpace::WeakKeyMap#[]= rejects integers as keys -fails:ObjectSpace::WeakKeyMap#[]= rejects floats as keys -fails:ObjectSpace::WeakKeyMap#[]= rejects booleans as keys -fails:ObjectSpace::WeakKeyMap#[]= rejects nil as keys diff --git a/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java b/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java index fed9bebea07..5e7e0ce02b0 100644 --- a/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java +++ b/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java @@ -21,9 +21,12 @@ import org.truffleruby.core.hash.HashingNodes.ToHashByHashCode; import org.truffleruby.core.klass.RubyClass; import org.truffleruby.core.proc.RubyProc; +import org.truffleruby.core.symbol.RubySymbol; import org.truffleruby.language.Nil; import org.truffleruby.annotations.Visibility; import com.oracle.truffle.api.dsl.Specialization; +import org.truffleruby.language.RubyGuards; +import org.truffleruby.language.control.RaiseException; import org.truffleruby.language.objects.AllocationTracing; import org.truffleruby.language.yield.CallBlockNode; @@ -94,13 +97,25 @@ boolean containsKey(RubyWeakKeyMap map, Object key, @CoreMethod(names = "[]=", required = 2) public abstract static class SetIndexNode extends CoreMethodArrayArgumentsNode { - @Specialization + @Specialization(guards = "isGarbageCollectable(key)") Object set(RubyWeakKeyMap map, Object key, Object value, @Cached ToHashByHashCode hashCodeNode) { int hashCode = hashCodeNode.execute(this, key); map.storage.put(new CompareByRubyHashEqlWrapper(key, hashCode), value); return value; } + + @Specialization(guards = "!isGarbageCollectable(key)") + void set(RubyWeakKeyMap map, Object key, Object value) { + throw new RaiseException( + getContext(), + coreExceptions().argumentError("WeakKeyMap must be garbage collectable", this)); + } + + boolean isGarbageCollectable(Object object) { + return !(object == nil || object instanceof Boolean || object instanceof RubySymbol || + RubyGuards.isRubyNumber(object)); + } } @CoreMethod(names = "clear") From 9def64f1beec00e75cc35e5da726ad7ad6a1cc8f Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Wed, 29 Jan 2025 20:44:36 +0200 Subject: [PATCH 4/6] Untag related MRI tests --- test/mri/excludes/TestWeakKeyMap.rb | 13 ------------- test/mri/excludes/TestWeakMap.rb | 1 - 2 files changed, 14 deletions(-) delete mode 100644 test/mri/excludes/TestWeakKeyMap.rb diff --git a/test/mri/excludes/TestWeakKeyMap.rb b/test/mri/excludes/TestWeakKeyMap.rb deleted file mode 100644 index 0fc31aadc89..00000000000 --- a/test/mri/excludes/TestWeakKeyMap.rb +++ /dev/null @@ -1,13 +0,0 @@ -exclude :test_aset_const, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_clear, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_clear_bug_20691, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_compaction, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_delete, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_frozen_object, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_gc_compact_stress, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_getkey, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_inconsistent_hash_key, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_inspect, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_key?, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_map, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" -exclude :test_no_hash_method, "NameError: uninitialized constant ObjectSpace::WeakKeyMap" diff --git a/test/mri/excludes/TestWeakMap.rb b/test/mri/excludes/TestWeakMap.rb index 53b540a57da..1bf5212eb17 100644 --- a/test/mri/excludes/TestWeakMap.rb +++ b/test/mri/excludes/TestWeakMap.rb @@ -1,2 +1 @@ -exclude :test_delete, "NoMethodError: undefined method `delete' for # => #>" exclude :test_inspect_garbage, "Expected /\\A\\#\\s\\#<(?:Object|collected):[^:<>]*+>(?:,|>\\z))+/ to match \"# => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #, # => #>\"." From 4cfbaf65d45db45f11254b4141b585b95d1a7dc3 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Mon, 3 Feb 2025 13:52:30 +0200 Subject: [PATCH 5/6] Add a test case to ensure a String key is not being frozen --- .../ruby/core/objectspace/weakkeymap/element_set_spec.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/spec/ruby/core/objectspace/weakkeymap/element_set_spec.rb b/spec/ruby/core/objectspace/weakkeymap/element_set_spec.rb index f08ae101b03..4e410b4ad7f 100644 --- a/spec/ruby/core/objectspace/weakkeymap/element_set_spec.rb +++ b/spec/ruby/core/objectspace/weakkeymap/element_set_spec.rb @@ -36,6 +36,15 @@ def should_accept(map, key, value) should_accept(map, y, x) end + it "does not duplicate and freeze String keys (like Hash#[]= does)" do + map = ObjectSpace::WeakKeyMap.new + key = +"a" + map[key] = 1 + + map.getkey("a").should.equal? key + map.getkey("a").should_not.frozen? + end + context "a key cannot be garbage collected" do it "raises ArgumentError when Integer is used as a key" do map = ObjectSpace::WeakKeyMap.new From 5bf1b5c0007807da60a519e49f3c68cd842d50cd Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Tue, 4 Feb 2025 14:14:22 +0200 Subject: [PATCH 6/6] Fix WeakKeyMap to prevent garbage collecting of keys --- .../collections/ConcurrentWeakKeysMap.java | 20 +++- .../collections/ConcurrentWeakSet.java | 5 +- .../hash/CompareByRubyHashEqlWrapper.java | 62 ---------- .../core/objectspace/WeakKeyMapNodes.java | 48 +++----- .../core/objectspace/WeakKeyMapStorage.java | 112 +++++++++++++++++- 5 files changed, 142 insertions(+), 105 deletions(-) delete mode 100644 src/main/java/org/truffleruby/core/hash/CompareByRubyHashEqlWrapper.java diff --git a/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java b/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java index dfc53d71220..523da2fdc4d 100644 --- a/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java +++ b/src/main/java/org/truffleruby/collections/ConcurrentWeakKeysMap.java @@ -24,7 +24,7 @@ * seed are fine as part of the pre-initialized context. */ public class ConcurrentWeakKeysMap { - protected final ConcurrentHashMap, Value> map = new ConcurrentHashMap<>(); + protected final ConcurrentHashMap, Value> map = new ConcurrentHashMap<>(); private final ReferenceQueue referenceQueue = new ReferenceQueue<>(); @TruffleBoundary @@ -39,20 +39,20 @@ public void clear() { @TruffleBoundary public boolean containsKey(Key key) { removeStaleEntries(); - return map.containsKey(new WeakKeyReference<>(key)); + return map.containsKey(buildWeakReference(key)); } @TruffleBoundary public Value get(Key key) { removeStaleEntries(); - return map.get(new WeakKeyReference<>(key)); + return map.get(buildWeakReference(key)); } /** Sets the value in the cache, always returns the old value. */ @TruffleBoundary public Value put(Key key, Value value) { removeStaleEntries(); - var ref = new WeakKeyReference<>(key, referenceQueue); + var ref = buildWeakReference(key, referenceQueue); return map.put(ref, value); } @@ -91,7 +91,15 @@ public Collection keys() { @TruffleBoundary public Value remove(Key key) { removeStaleEntries(); - return map.remove(new WeakKeyReference<>(key)); + return map.remove(buildWeakReference(key)); + } + + protected WeakReference buildWeakReference(Key key) { + return new WeakKeyReference<>(key); + } + + protected WeakReference buildWeakReference(Key key, ReferenceQueue referenceQueue) { + return new WeakKeyReference<>(key, referenceQueue); } /** Attempts to remove map entries whose values have been made unreachable by the GC. @@ -99,7 +107,7 @@ public Value remove(Key key) { * This relies on the underlying {@link WeakReference} instance being enqueued to the {@link #referenceQueue} queue. * It is possible that the map still contains {@link WeakReference} instances whose key has been nulled out after a * call to this method (the reference not having been enqueued yet)! */ - private void removeStaleEntries() { + protected void removeStaleEntries() { WeakKeyReference ref; while ((ref = (WeakKeyReference) referenceQueue.poll()) != null) { // Here ref.get() is null, so it will not remove a new key-value pair with the same key diff --git a/src/main/java/org/truffleruby/collections/ConcurrentWeakSet.java b/src/main/java/org/truffleruby/collections/ConcurrentWeakSet.java index efbbfc3c1ca..898d47388cb 100644 --- a/src/main/java/org/truffleruby/collections/ConcurrentWeakSet.java +++ b/src/main/java/org/truffleruby/collections/ConcurrentWeakSet.java @@ -9,6 +9,7 @@ */ package org.truffleruby.collections; +import java.lang.ref.WeakReference; import java.util.Iterator; import java.util.NoSuchElementException; @@ -41,10 +42,10 @@ public WeakSetIterator iterator() { } private static final class WeakSetIterator implements Iterator { - private final Iterator> keysIterator; + private final Iterator> keysIterator; private E nextElement; - private WeakSetIterator(Iterator> keysIterator) { + private WeakSetIterator(Iterator> keysIterator) { this.keysIterator = keysIterator; computeNext(); } diff --git a/src/main/java/org/truffleruby/core/hash/CompareByRubyHashEqlWrapper.java b/src/main/java/org/truffleruby/core/hash/CompareByRubyHashEqlWrapper.java deleted file mode 100644 index 4f50212987c..00000000000 --- a/src/main/java/org/truffleruby/core/hash/CompareByRubyHashEqlWrapper.java +++ /dev/null @@ -1,62 +0,0 @@ -/* - * Copyright (c) 2013, 2025 Oracle and/or its affiliates. All rights reserved. This - * code is released under a tri EPL/GPL/LGPL license. You can use it, - * redistribute it and/or modify it under the terms of the: - * - * Eclipse Public License version 2.0, or - * GNU General Public License version 2, or - * GNU Lesser General Public License version 2.1. - */ -package org.truffleruby.core.hash; - -import org.truffleruby.core.hash.HashingNodes.ToHashByHashCode; -import org.truffleruby.core.kernel.KernelNodes.SameOrEqlNode; - -/** Wraps a key so that it will be hashed and compared according to Ruby Hash key semantics, using #hash and #eql?. */ -public class CompareByRubyHashEqlWrapper { - - private final Object key; - private final int hashCode; - - /** @param hashCode the result of {@link ToHashByHashCode} on the key */ - public CompareByRubyHashEqlWrapper(Object key, int hashCode) { - this.key = key; - this.hashCode = hashCode; - } - - @Override - public int hashCode() { - return hashCode; - } - - @Override - public boolean equals(Object obj) { - // We use an uncached SameOrEqlNode here since we are not in PE-able code - return obj instanceof CompareByRubyHashEqlWrapper other && hashCode == other.hashCode && - SameOrEqlNode.executeUncached(key, other.key); - } - - public static final class RecordingCompareByRubyHashEqlWrapper extends CompareByRubyHashEqlWrapper { - - public Object keyInMap; - - public RecordingCompareByRubyHashEqlWrapper(Object key, int hashCode) { - super(key, hashCode); - } - - @Override - public int hashCode() { - return super.hashCode(); - } - - @Override - public boolean equals(Object obj) { - if (super.equals(obj)) { - this.keyInMap = ((CompareByRubyHashEqlWrapper) obj).key; - return true; - } else { - return false; - } - } - } -} diff --git a/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java b/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java index 5e7e0ce02b0..a17f8133e84 100644 --- a/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java +++ b/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapNodes.java @@ -16,9 +16,6 @@ import org.truffleruby.annotations.Primitive; import org.truffleruby.builtins.CoreMethodArrayArgumentsNode; import org.truffleruby.builtins.PrimitiveArrayArgumentsNode; -import org.truffleruby.core.hash.CompareByRubyHashEqlWrapper; -import org.truffleruby.core.hash.CompareByRubyHashEqlWrapper.RecordingCompareByRubyHashEqlWrapper; -import org.truffleruby.core.hash.HashingNodes.ToHashByHashCode; import org.truffleruby.core.klass.RubyClass; import org.truffleruby.core.proc.RubyProc; import org.truffleruby.core.symbol.RubySymbol; @@ -30,8 +27,6 @@ import org.truffleruby.language.objects.AllocationTracing; import org.truffleruby.language.yield.CallBlockNode; -import java.util.Objects; - @CoreModule(value = "ObjectSpace::WeakKeyMap", isClass = true) public abstract class WeakKeyMapNodes { @@ -59,10 +54,8 @@ int size(RubyWeakKeyMap map) { public abstract static class GetIndexNode extends CoreMethodArrayArgumentsNode { @Specialization - Object get(RubyWeakKeyMap map, Object key, - @Cached ToHashByHashCode hashCodeNode) { - int hashCode = hashCodeNode.execute(this, key); - Object value = map.storage.get(new CompareByRubyHashEqlWrapper(key, hashCode)); + Object get(RubyWeakKeyMap map, Object key) { + Object value = map.storage.get(key); return value == null ? nil : value; } } @@ -71,12 +64,11 @@ Object get(RubyWeakKeyMap map, Object key, public abstract static class GetKeyNode extends CoreMethodArrayArgumentsNode { @Specialization - Object getKey(RubyWeakKeyMap map, Object key, - @Cached ToHashByHashCode hashCodeNode) { - int hashCode = hashCodeNode.execute(this, key); - var recordingWrapper = new RecordingCompareByRubyHashEqlWrapper(key, hashCode); - if (map.storage.containsKey(recordingWrapper)) { - return Objects.requireNonNull(recordingWrapper.keyInMap); + Object getKey(RubyWeakKeyMap map, Object key) { + Object originalKey = map.storage.getKey(key); + + if (originalKey != null) { + return originalKey; } else { return nil; } @@ -87,10 +79,8 @@ Object getKey(RubyWeakKeyMap map, Object key, public abstract static class MemberNode extends CoreMethodArrayArgumentsNode { @Specialization - boolean containsKey(RubyWeakKeyMap map, Object key, - @Cached ToHashByHashCode hashCodeNode) { - int hashCode = hashCodeNode.execute(this, key); - return map.storage.containsKey(new CompareByRubyHashEqlWrapper(key, hashCode)); + boolean containsKey(RubyWeakKeyMap map, Object key) { + return map.storage.containsKey(key); } } @@ -98,15 +88,13 @@ boolean containsKey(RubyWeakKeyMap map, Object key, public abstract static class SetIndexNode extends CoreMethodArrayArgumentsNode { @Specialization(guards = "isGarbageCollectable(key)") - Object set(RubyWeakKeyMap map, Object key, Object value, - @Cached ToHashByHashCode hashCodeNode) { - int hashCode = hashCodeNode.execute(this, key); - map.storage.put(new CompareByRubyHashEqlWrapper(key, hashCode), value); + Object set(RubyWeakKeyMap map, Object key, Object value) { + map.storage.put(key, value); return value; } @Specialization(guards = "!isGarbageCollectable(key)") - void set(RubyWeakKeyMap map, Object key, Object value) { + void setWithInvalidKey(RubyWeakKeyMap map, Object key, Object value) { throw new RaiseException( getContext(), coreExceptions().argumentError("WeakKeyMap must be garbage collectable", this)); @@ -134,10 +122,8 @@ public abstract static class DeleteNode extends CoreMethodArrayArgumentsNode { @Specialization Object delete(RubyWeakKeyMap map, Object key, RubyProc block, @Cached InlinedConditionProfile isContainedProfile, - @Cached CallBlockNode yieldNode, - @Cached ToHashByHashCode hashCodeNode) { - int hashCode = hashCodeNode.execute(this, key); - Object value = map.storage.remove(new CompareByRubyHashEqlWrapper(key, hashCode)); + @Cached CallBlockNode yieldNode) { + Object value = map.storage.remove(key); if (isContainedProfile.profile(this, value != null)) { return value; @@ -148,10 +134,8 @@ Object delete(RubyWeakKeyMap map, Object key, RubyProc block, @Specialization Object delete(RubyWeakKeyMap map, Object key, Nil block, - @Cached InlinedConditionProfile isContainedProfile, - @Cached ToHashByHashCode hashCodeNode) { - int hashCode = hashCodeNode.execute(this, key); - Object value = map.storage.remove(new CompareByRubyHashEqlWrapper(key, hashCode)); + @Cached InlinedConditionProfile isContainedProfile) { + Object value = map.storage.remove(key); if (isContainedProfile.profile(this, value != null)) { return value; diff --git a/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapStorage.java b/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapStorage.java index d6fe0682200..55e028c0b06 100644 --- a/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapStorage.java +++ b/src/main/java/org/truffleruby/core/objectspace/WeakKeyMapStorage.java @@ -9,9 +9,115 @@ */ package org.truffleruby.core.objectspace; +import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary; import org.truffleruby.collections.ConcurrentWeakKeysMap; -import org.truffleruby.core.hash.CompareByRubyHashEqlWrapper; +import org.truffleruby.core.hash.HashingNodes.ToHashByHashCode; +import org.truffleruby.core.kernel.KernelNodes.SameOrEqlNode; + +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.Objects; + +public final class WeakKeyMapStorage extends ConcurrentWeakKeysMap { + + @TruffleBoundary + public Object getKey(Object key) { + removeStaleEntries(); + + int hashCode = ToHashByHashCode.executeUncached(key); + var ref = new RecordingCompareByRubyHashEqlWeakReference(key, hashCode); + + if (map.containsKey(ref)) { + return Objects.requireNonNull(ref.keyInMap); + } else { + return null; + } + } + + @Override + protected WeakReference buildWeakReference(Object key) { + int hashCode = ToHashByHashCode.executeUncached(key); + return new CompareByRubyHashEqlWeakReference(key, hashCode); + } + + @Override + protected WeakReference buildWeakReference(Object key, ReferenceQueue referenceQueue) { + int hashCode = ToHashByHashCode.executeUncached(key); + return new CompareByRubyHashEqlWeakReference(key, referenceQueue, hashCode); + } + + /** Wraps a key so that it will be hashed and compared according to Ruby Hash key semantics, using #hash and + * #eql?. */ + private static class CompareByRubyHashEqlWeakReference extends WeakReference { + + private final int hashCode; + + /** @param hashCode the result of {@link ToHashByHashCode} on the key */ + public CompareByRubyHashEqlWeakReference(Object key, int hashCode) { + super(key); + Objects.requireNonNull(key); + this.hashCode = hashCode; + } + + /** @param hashCode the result of {@link ToHashByHashCode} on the key */ + public CompareByRubyHashEqlWeakReference(Object key, ReferenceQueue queue, int hashCode) { + super(key, queue); + Objects.requireNonNull(key); + this.hashCode = hashCode; + } + + @Override + public int hashCode() { + return hashCode; + } + + @Override + public boolean equals(Object other) { + if (this == other) { + return true; + } + + if (other instanceof CompareByRubyHashEqlWeakReference ref) { + Object key = get(); + Object otherKey = ref.get(); + + // We use an uncached SameOrEqlNode here since we are not in PE-able code + return key != null && otherKey != null && SameOrEqlNode.executeUncached(key, otherKey); + } else { + return false; + } + } + } + + private static final class RecordingCompareByRubyHashEqlWeakReference extends CompareByRubyHashEqlWeakReference { + + public Object keyInMap; + + public RecordingCompareByRubyHashEqlWeakReference(Object key, int hashCode) { + super(key, hashCode); + } + + public RecordingCompareByRubyHashEqlWeakReference( + Object key, + ReferenceQueue queue, + int hashCode) { + super(key, queue, hashCode); + } + + @Override + public int hashCode() { + return super.hashCode(); + } + + @Override + public boolean equals(Object other) { + if (super.equals(other)) { + this.keyInMap = ((CompareByRubyHashEqlWeakReference) other).get(); + return true; + } else { + return false; + } + } + } -// An alias of ConcurrentWeakKeysMap to keep things short -public final class WeakKeyMapStorage extends ConcurrentWeakKeysMap { }