Skip to content

Commit

Permalink
[Truffle] Hash#merge! works except for an ordering issue.
Browse files Browse the repository at this point in the history
  • Loading branch information
@chrisseaton
chrisseaton committed Mar 3, 2015
1 parent 3e334e0 commit 68a6a35
Show file tree
Hide file tree
Showing 9 changed files with 101 additions and 25 deletions.
11 changes: 0 additions & 11 deletions spec/truffle/tags/core/hash/merge_tags.txt
View file
@@ -1,15 +1,4 @@
fails:Hash#merge returns a new hash by combining self with the contents of other
fails:Hash#merge sets any duplicate key to the value of block if passed a block
fails:Hash#merge tries to convert the passed argument to a hash using #to_hash
fails:Hash#merge does not call to_hash on hash subclasses
fails:Hash#merge returns subclass instance for subclasses
fails:Hash#merge processes entries with same order as each()
fails:Hash#merge! adds the entries from other, overwriting duplicate keys. Returns self
fails:Hash#merge! sets any duplicate key to the value of block if passed a block
fails:Hash#merge! tries to convert the passed argument to a hash using #to_hash
fails:Hash#merge! does not call to_hash on hash subclasses
fails:Hash#merge! processes entries with same order as merge()
fails:Hash#merge! raises a RuntimeError on a frozen instance that is modified
fails:Hash#merge! checks frozen status before coercing an object with #to_hash
fails:Hash#merge! raises a RuntimeError on a frozen instance that would not be modified
fails:Hash#merge! does not raise an exception if changing the value of an existing key during iteration
3 changes: 3 additions & 0 deletions truffle/src/main/java/org/jruby/truffle/nodes/core/CoreMethod.java
View file
Expand Up @@ -40,6 +40,9 @@

boolean needsSelf() default true;

// TODO CS 3-Mar-15 needsSelf() gets ignored in some cases - see CoreMethodNodeManager#addMethod
boolean reallyDoesNeedSelf() default false;

int required() default 0;

int optional() default 0;
Expand Down
2 changes: 1 addition & 1 deletion truffle/src/main/java/org/jruby/truffle/nodes/core/CoreMethodNodeManager.java
View file
Expand Up @@ -101,7 +101,7 @@ private static void addMethod(RubyClass rubyObjectClass, MethodDetails methodDet

// Do not use needsSelf=true in module functions, it is either the module/class or the instance.
// Usage of needsSelf is quite rare for singleton methods (except constructors).
final boolean needsSelf = !anno.isModuleFunction() && !anno.onSingleton() && anno.needsSelf();
final boolean needsSelf = !anno.isModuleFunction() && !anno.onSingleton() && anno.needsSelf() || anno.reallyDoesNeedSelf();

final RubyRootNode rootNode = makeGenericMethod(context, methodDetails, needsSelf);

Expand Down
25 changes: 15 additions & 10 deletions truffle/src/main/java/org/jruby/truffle/nodes/core/HashNodes.java
View file
Expand Up @@ -102,7 +102,7 @@ public boolean equalNonHash(RubyHash a, Object b) {

}

@CoreMethod(names = "[]", onSingleton = true, argumentsAsArray = true)
@CoreMethod(names = "[]", onSingleton = true, argumentsAsArray = true, reallyDoesNeedSelf = true)
public abstract static class ConstructNode extends HashCoreMethodNode {

private final BranchProfile singleObject = BranchProfile.create();
Expand All @@ -112,6 +112,7 @@ public abstract static class ConstructNode extends HashCoreMethodNode {
private final BranchProfile largePackedArray = BranchProfile.create();
private final BranchProfile otherArray = BranchProfile.create();
private final BranchProfile singleOther = BranchProfile.create();
private final BranchProfile otherHash = BranchProfile.create();
private final BranchProfile keyValues = BranchProfile.create();

public ConstructNode(RubyContext context, SourceSection sourceSection) {
Expand All @@ -124,7 +125,7 @@ public ConstructNode(ConstructNode prev) {

@ExplodeLoop
@Specialization
public RubyHash construct(Object[] args) {
public RubyHash construct(RubyClass hashClass, Object[] args) {
if (args.length == 1) {
singleObject.enter();

Expand Down Expand Up @@ -171,7 +172,7 @@ public RubyHash construct(Object[] args) {
}
}

return new RubyHash(getContext().getCoreLibrary().getHashClass(), null, null, newStore, size, null);
return new RubyHash(hashClass, null, null, newStore, size, null);
} else {
largePackedArray.enter();

Expand All @@ -198,12 +199,16 @@ public RubyHash construct(Object[] args) {
keyValues.add(new KeyValue(pairStore[0], pairStore[1]));
}

return HashOperations.verySlowFromEntries(getContext(), keyValues);
return HashOperations.verySlowFromEntries(hashClass, keyValues);
}
} else {
otherArray.enter();
throw new UnsupportedOperationException("other array");
}
} else if (arg instanceof RubyHash) {
otherHash.enter();

return HashOperations.verySlowFromEntries(hashClass, HashOperations.verySlowToKeyValues((RubyHash) arg));
} else {
singleOther.enter();
throw new UnsupportedOperationException("single other");
Expand All @@ -217,7 +222,7 @@ public RubyHash construct(Object[] args) {
entries.add(new KeyValue(args[n], args[n + 1]));
}

return HashOperations.verySlowFromEntries(getContext(), entries);
return HashOperations.verySlowFromEntries(hashClass, entries);
}
}

Expand Down Expand Up @@ -947,7 +952,7 @@ public RubyHash mergePackedArrayNull(RubyHash hash, RubyHash other) {
final Object[] store = (Object[]) hash.getStore();
final Object[] copy = Arrays.copyOf(store, HashOperations.SMALL_HASH_SIZE * 2);

return new RubyHash(getContext().getCoreLibrary().getHashClass(), hash.getDefaultBlock(), hash.getDefaultValue(), copy, hash.getSize(), null);
return new RubyHash(hash.getLogicalClass(), hash.getDefaultBlock(), hash.getDefaultValue(), copy, hash.getSize(), null);
}

@ExplodeLoop
Expand Down Expand Up @@ -987,14 +992,14 @@ public RubyHash mergePackedArrayPackedArray(VirtualFrame frame, RubyHash hash, R

if (mergeFromACount == 0) {
nothingFromFirstProfile.enter();
return new RubyHash(getContext().getCoreLibrary().getHashClass(), hash.getDefaultBlock(), hash.getDefaultValue(), Arrays.copyOf(storeB, HashOperations.SMALL_HASH_SIZE * 2), storeBSize, null);
return new RubyHash(hash.getLogicalClass(), hash.getDefaultBlock(), hash.getDefaultValue(), Arrays.copyOf(storeB, HashOperations.SMALL_HASH_SIZE * 2), storeBSize, null);
}

considerNothingFromSecondProfile.enter();

if (mergeFromACount == storeB.length) {
nothingFromSecondProfile.enter();
return new RubyHash(getContext().getCoreLibrary().getHashClass(), hash.getDefaultBlock(), hash.getDefaultValue(), Arrays.copyOf(storeB, HashOperations.SMALL_HASH_SIZE * 2), storeBSize, null);
return new RubyHash(hash.getLogicalClass(), hash.getDefaultBlock(), hash.getDefaultValue(), Arrays.copyOf(storeB, HashOperations.SMALL_HASH_SIZE * 2), storeBSize, null);
}

considerResultIsSmallProfile.enter();
Expand Down Expand Up @@ -1022,7 +1027,7 @@ public RubyHash mergePackedArrayPackedArray(VirtualFrame frame, RubyHash hash, R
index += 2;
}

return new RubyHash(getContext().getCoreLibrary().getHashClass(), hash.getDefaultBlock(), hash.getDefaultValue(), merged, mergedSize, null);
return new RubyHash(hash.getLogicalClass(), hash.getDefaultBlock(), hash.getDefaultValue(), merged, mergedSize, null);
}

CompilerDirectives.transferToInterpreter();
Expand All @@ -1032,7 +1037,7 @@ public RubyHash mergePackedArrayPackedArray(VirtualFrame frame, RubyHash hash, R

@Specialization
public RubyHash mergeBucketsBuckets(RubyHash hash, RubyHash other) {
final RubyHash merged = new RubyHash(getContext().getCoreLibrary().getHashClass(), null, null, new Entry[HashOperations.capacityGreaterThan(hash.getSize() + other.getSize())], 0, null);
final RubyHash merged = new RubyHash(hash.getLogicalClass(), null, null, new Entry[HashOperations.capacityGreaterThan(hash.getSize() + other.getSize())], 0, null);

int size = 0;

Expand Down
9 changes: 7 additions & 2 deletions truffle/src/main/java/org/jruby/truffle/runtime/hash/HashOperations.java
View file
Expand Up @@ -13,6 +13,7 @@
import org.jruby.truffle.nodes.RubyNode;
import org.jruby.truffle.runtime.DebugOperations;
import org.jruby.truffle.runtime.RubyContext;
import org.jruby.truffle.runtime.core.RubyClass;
import org.jruby.truffle.runtime.core.RubyHash;
import org.jruby.truffle.runtime.core.RubyString;
import org.jruby.util.cli.Options;
Expand All @@ -38,11 +39,15 @@ public static int capacityGreaterThan(int size) {
return CAPACITIES[CAPACITIES.length - 1];
}

@CompilerDirectives.TruffleBoundary
public static RubyHash verySlowFromEntries(RubyContext context, List<KeyValue> entries) {
return verySlowFromEntries(context.getCoreLibrary().getHashClass(), entries);
}

@CompilerDirectives.TruffleBoundary
public static RubyHash verySlowFromEntries(RubyClass hashClass, List<KeyValue> entries) {
RubyNode.notDesignedForCompilation();

final RubyHash hash = new RubyHash(context.getCoreLibrary().getHashClass(), null, null, null, 0, null);
final RubyHash hash = new RubyHash(hashClass, null, null, null, 0, null);
verySlowSetKeyValues(hash, entries);
return hash;
}
Expand Down
1 change: 1 addition & 0 deletions truffle/src/main/ruby/core.rb
View file
Expand Up @@ -40,6 +40,7 @@
require_relative 'core/rubinius/common/exception'
require_relative 'core/rubinius/common/undefined'
require_relative 'core/rubinius/common/type'
require_relative 'core/rubinius/common/hash'
require_relative 'core/rubinius/common/array'
require_relative 'core/rubinius/common/kernel'
require_relative 'core/rubinius/common/identity_map'
Expand Down
13 changes: 13 additions & 0 deletions truffle/src/main/ruby/core/hash.rb
View file
Expand Up @@ -20,4 +20,17 @@ def to_hash
self
end

# Implementation of a fundamental Rubinius method that allows their Hash
# implementation to work. We probably want to remove uses of this in the long
# term, as it creates objects which already exist for them and we have to
# create, but for now it allows us to use more Rubinius code unmodified.

KeyValue = Struct.new(:key, :value)

def each_item
each do |key, value|
yield KeyValue.new(key, value)
end
end

end
2 changes: 1 addition & 1 deletion truffle/src/main/ruby/core/rubinius/common/float.rb
View file
Expand Up @@ -24,7 +24,7 @@
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

# Only part of Rubinius' kernel.rb
# Only part of Rubinius' float.rb

class Float < Numeric

Expand Down
60 changes: 60 additions & 0 deletions truffle/src/main/ruby/core/rubinius/common/hash.rb
View file
@@ -0,0 +1,60 @@
# Copyright (c) 2007-2014, Evan Phoenix and contributors
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are met:
#
# * Redistributions of source code must retain the above copyright notice, this
# list of conditions and the following disclaimer.
# * Redistributions in binary form must reproduce the above copyright notice
# this list of conditions and the following disclaimer in the documentation
# and/or other materials provided with the distribution.
# * Neither the name of Rubinius nor the names of its contributors
# may be used to endorse or promote products derived from this software
# without specific prior written permission.
#
# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE
# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

# Only part of Rubinius' hash.rb

class Hash

alias_method :store, :[]=

# Used internally to get around subclasses redefining #[]=
alias_method :__store__, :[]=

def merge!(other)
Rubinius.check_frozen

other = Rubinius::Type.coerce_to other, Hash, :to_hash

if block_given?
other.each_item do |item|
key = item.key
if key? key
__store__ key, yield(key, self[key], item.value)
else
__store__ key, item.value
end
end
else
other.each_item do |item|
__store__ item.key, item.value
end
end
self
end

alias_method :update, :merge!

end

0 comments on commit 68a6a35

Please sign in to comment.