Skip to content

Commit 2fe521f

Browse files
aardvark179aardvark179
committed
[GR-15414] Hash merge fix
PullRequest: truffleruby/815
2 parents 93a102d + 6dc937f commit 2fe521f

File tree

4 files changed

+24
-229
lines changed

4 files changed

+24
-229
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ Bug fixes:
1313
Compatibility
1414

1515
* `String#-@` now performs string deduplication
16+
* `Hash#merge` now preserves the key order from the original hash for merged values (#1650).
1617

1718
# 1.0 RC 17
1819

‎spec/ruby/core/hash/merge_spec.rb

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,24 @@
6363
merge_pairs.should == each_pairs
6464
end
6565

66+
it "preserves the order of merged elements" do
67+
h1 = { 1 => 2, 3 => 4, 5 => 6 }
68+
h2 = { 1 => 7 }
69+
merge_pairs = []
70+
h1.merge(h2).each_pair { |k, v| merge_pairs << [k, v] }
71+
merge_pairs.should == [[1,7], [3, 4], [5, 6]]
72+
end
73+
74+
it "preserves the order of merged elements for large hashes" do
75+
h1 = {}
76+
h2 = {}
77+
merge_pairs = []
78+
expected_pairs = []
79+
(1..100).each { |x| h1[x] = x; h2[101 - x] = x; expected_pairs << [x, 101 - x] }
80+
h1.merge(h2).each_pair { |k, v| merge_pairs << [k, v] }
81+
merge_pairs.should == expected_pairs
82+
end
83+
6684
ruby_version_is "2.6" do
6785
it "accepts multiple hashes" do
6886
result = { a: 1 }.merge({ b: 2 }, { c: 3 }, { d: 4 })

‎src/main/java/org/truffleruby/core/hash/HashNodes.java

Lines changed: 0 additions & 228 deletions
Original file line numberDiff line numberDiff line change
@@ -688,234 +688,6 @@ private Object yieldPair(DynamicObject block, Object key, Object value) {
688688

689689
}
690690

691-
@ImportStatic(HashGuards.class)
692-
@Primitive(name = "hash_merge", needsSelf = false)
693-
public abstract static class MergeNode extends PrimitiveArrayArgumentsNode {
694-
695-
@Child private LookupEntryNode lookupEntryNode;
696-
@Child private SetNode setNode = SetNode.create();
697-
@Child private AllocateObjectNode allocateObjectNode = AllocateObjectNode.create();
698-
@Child private CompareHashKeysNode compareHashKeysNode = new CompareHashKeysNode();
699-
700-
// Merge with an empty hash, without a block
701-
702-
@Specialization(guards = {"isNullHash(hash)", "isRubyHash(other)", "isNullHash(other)"})
703-
public DynamicObject mergeEmptyEmpty(DynamicObject hash, DynamicObject other) {
704-
return newHash(hash, null, 0, Layouts.HASH.getCompareByIdentity(hash));
705-
}
706-
707-
@Specialization(guards = {"isEmptyHash(hash)", "isRubyHash(other)", "isPackedHash(other)"})
708-
public DynamicObject mergeEmptyPacked(DynamicObject hash, DynamicObject other) {
709-
final Object[] store = (Object[]) Layouts.HASH.getStore(other);
710-
final Object[] copy = PackedArrayStrategy.copyStore(getContext(), store);
711-
return newHash(hash, copy, Layouts.HASH.getSize(other), Layouts.HASH.getCompareByIdentity(hash));
712-
}
713-
714-
@Specialization(guards = {"isPackedHash(hash)", "isRubyHash(other)", "isEmptyHash(other)"})
715-
public DynamicObject mergePackedEmpty(DynamicObject hash, DynamicObject other) {
716-
return mergeEmptyPacked(other, hash);
717-
}
718-
719-
@Specialization(guards = {"isEmptyHash(hash)", "isRubyHash(other)", "isBucketHash(other)"})
720-
public DynamicObject mergeEmptyBuckets(DynamicObject hash, DynamicObject other) {
721-
final DynamicObject merged = newHash(hash, null, 0, Layouts.HASH.getCompareByIdentity(hash));
722-
BucketsStrategy.copyInto(getContext(), other, merged);
723-
return merged;
724-
}
725-
726-
@Specialization(guards = {"isBucketHash(hash)", "isRubyHash(other)", "isEmptyHash(other)"})
727-
public DynamicObject mergeBucketsEmpty(DynamicObject hash, DynamicObject other) {
728-
return mergeEmptyBuckets(other, hash);
729-
}
730-
731-
// Merge non-empty packed with non-empty packed, without a block
732-
733-
@Specialization(guards = {"isPackedHash(hash)", "!isEmptyHash(hash)", "isRubyHash(other)", "isPackedHash(other)", "!isEmptyHash(other)"})
734-
public DynamicObject mergePackedPacked(DynamicObject hash, DynamicObject other,
735-
@Cached("createBinaryProfile()") ConditionProfile byIdentityProfile,
736-
@Cached("createBinaryProfile()") ConditionProfile nothingFromFirstProfile,
737-
@Cached("createBinaryProfile()") ConditionProfile resultIsSmallProfile) {
738-
assert HashOperations.verifyStore(getContext(), hash);
739-
assert HashOperations.verifyStore(getContext(), other);
740-
741-
final boolean compareByIdentity = byIdentityProfile.profile(Layouts.HASH.getCompareByIdentity(hash));
742-
743-
final Object[] storeA = (Object[]) Layouts.HASH.getStore(hash);
744-
final int storeASize = Layouts.HASH.getSize(hash);
745-
746-
final Object[] storeB = (Object[]) Layouts.HASH.getStore(other);
747-
final int storeBSize = Layouts.HASH.getSize(other);
748-
749-
// Go through and figure out what gets merged from each hash
750-
751-
final boolean[] mergeFromA = new boolean[storeASize];
752-
final int mergeFromACount = mergedPackedHashes(compareByIdentity, storeA, storeASize, storeB, storeBSize, mergeFromA);
753-
754-
// If nothing comes from A, it's easy
755-
756-
if (nothingFromFirstProfile.profile(mergeFromACount == 0)) {
757-
return newHash(hash, PackedArrayStrategy.copyStore(getContext(), storeB), storeBSize, compareByIdentity);
758-
}
759-
760-
// More complicated case where some things from each hash, but it still fits in a packed
761-
// array
762-
763-
final int mergedSize = storeBSize + mergeFromACount;
764-
765-
if (resultIsSmallProfile.profile(storeBSize + mergeFromACount <= getContext().getOptions().HASH_PACKED_ARRAY_MAX)) {
766-
final Object[] merged = PackedArrayStrategy.createStore(getContext());
767-
768-
int index = 0;
769-
770-
for (int n = 0; n < storeASize; n++) {
771-
if (mergeFromA[n]) {
772-
PackedArrayStrategy.setHashedKeyValue(merged, index,
773-
PackedArrayStrategy.getHashed(storeA, n),
774-
PackedArrayStrategy.getKey(storeA, n),
775-
PackedArrayStrategy.getValue(storeA, n));
776-
index++;
777-
}
778-
}
779-
780-
for (int n = 0; n < storeBSize; n++) {
781-
PackedArrayStrategy.setHashedKeyValue(merged, index,
782-
PackedArrayStrategy.getHashed(storeB, n),
783-
PackedArrayStrategy.getKey(storeB, n),
784-
PackedArrayStrategy.getValue(storeB, n));
785-
index++;
786-
}
787-
788-
return newHash(hash, merged, mergedSize, compareByIdentity);
789-
}
790-
791-
// Most complicated cases where things from both hashes, and it also needs to be
792-
// promoted to buckets
793-
794-
final Entry[] newStore = new Entry[BucketsStrategy.capacityGreaterThan(mergedSize)];
795-
final DynamicObject merged = newHash(hash, newStore, 0, compareByIdentity);
796-
797-
for (int n = 0; n < storeASize; n++) {
798-
if (mergeFromA[n]) {
799-
setNode.executeSet(merged, PackedArrayStrategy.getKey(storeA, n), PackedArrayStrategy.getValue(storeA, n), false);
800-
}
801-
}
802-
803-
for (int n = 0; n < storeBSize; n++) {
804-
setNode.executeSet(merged, PackedArrayStrategy.getKey(storeB, n), PackedArrayStrategy.getValue(storeB, n), false);
805-
}
806-
807-
assert HashOperations.verifyStore(getContext(), hash);
808-
return merged;
809-
}
810-
811-
@ExplodeLoop
812-
private int mergedPackedHashes(boolean compareByIdentity, Object[] storeA, int storeASize, Object[] storeB,
813-
int storeBSize, boolean[] mergeFromA) {
814-
int mergeFromACount = 0;
815-
for (int a = 0; a < getContext().getOptions().HASH_PACKED_ARRAY_MAX; a++) {
816-
if (a < storeASize) {
817-
boolean merge = true;
818-
819-
for (int b = 0; b < getContext().getOptions().HASH_PACKED_ARRAY_MAX; b++) {
820-
if (b < storeBSize) {
821-
if (equalKeys(compareByIdentity, PackedArrayStrategy.getKey(storeA, a),
822-
PackedArrayStrategy.getHashed(storeA, a), PackedArrayStrategy.getKey(storeB, b),
823-
PackedArrayStrategy.getHashed(storeB, b))) {
824-
merge = false;
825-
break;
826-
}
827-
}
828-
}
829-
830-
if (merge) {
831-
mergeFromACount++;
832-
}
833-
834-
mergeFromA[a] = merge;
835-
}
836-
}
837-
return mergeFromACount;
838-
}
839-
840-
// Merge non-empty buckets with non-empty buckets, without a block
841-
842-
@Specialization(guards = {"isBucketHash(hash)", "!isEmptyHash(hash)", "isRubyHash(other)", "isBucketHash(other)", "!isEmptyHash(other)"})
843-
public DynamicObject mergeBucketsBuckets(DynamicObject hash, DynamicObject other) {
844-
final boolean compareByIdentity = Layouts.HASH.getCompareByIdentity(hash);
845-
final Entry[] newStore = new Entry[BucketsStrategy.capacityGreaterThan(Layouts.HASH.getSize(hash) + Layouts.HASH.getSize(other))];
846-
final DynamicObject merged = newHash(hash, newStore, 0, compareByIdentity);
847-
848-
for (KeyValue keyValue : BucketsStrategy.iterableKeyValues(Layouts.HASH.getFirstInSequence(hash))) {
849-
setNode.executeSet(merged, keyValue.getKey(), keyValue.getValue(), compareByIdentity);
850-
}
851-
852-
for (KeyValue keyValue : BucketsStrategy.iterableKeyValues(Layouts.HASH.getFirstInSequence(other))) {
853-
setNode.executeSet(merged, keyValue.getKey(), keyValue.getValue(), compareByIdentity);
854-
}
855-
856-
assert HashOperations.verifyStore(getContext(), hash);
857-
return merged;
858-
}
859-
860-
// Merge combinations of packed and buckets, without a block
861-
862-
@Specialization(guards = {"isPackedHash(hash)", "!isEmptyHash(hash)", "isRubyHash(other)", "isBucketHash(other)", "!isEmptyHash(other)"})
863-
public DynamicObject mergePackedBuckets(DynamicObject hash, DynamicObject other) {
864-
final boolean compareByIdentity = Layouts.HASH.getCompareByIdentity(hash);
865-
final Entry[] newStore = new Entry[BucketsStrategy.capacityGreaterThan(Layouts.HASH.getSize(hash) + Layouts.HASH.getSize(other))];
866-
final DynamicObject merged = newHash(hash, newStore, 0, compareByIdentity);
867-
868-
final Object[] hashStore = (Object[]) Layouts.HASH.getStore(hash);
869-
final int hashSize = Layouts.HASH.getSize(hash);
870-
871-
for (int n = 0; n < getContext().getOptions().HASH_PACKED_ARRAY_MAX; n++) {
872-
if (n < hashSize) {
873-
setNode.executeSet(merged, PackedArrayStrategy.getKey(hashStore, n), PackedArrayStrategy.getValue(hashStore, n), compareByIdentity);
874-
}
875-
}
876-
877-
for (KeyValue keyValue : BucketsStrategy.iterableKeyValues(Layouts.HASH.getFirstInSequence(other))) {
878-
setNode.executeSet(merged, keyValue.getKey(), keyValue.getValue(), compareByIdentity);
879-
}
880-
881-
assert HashOperations.verifyStore(getContext(), hash);
882-
return merged;
883-
}
884-
885-
@Specialization(guards = {"isBucketHash(hash)", "!isEmptyHash(hash)", "isRubyHash(other)", "isPackedHash(other)", "!isEmptyHash(other)"})
886-
public DynamicObject mergeBucketsPacked(DynamicObject hash, DynamicObject other) {
887-
final boolean compareByIdentity = Layouts.HASH.getCompareByIdentity(hash);
888-
final Entry[] newStore = new Entry[BucketsStrategy.capacityGreaterThan(Layouts.HASH.getSize(hash) + Layouts.HASH.getSize(other))];
889-
final DynamicObject merged = newHash(hash, newStore, 0, compareByIdentity);
890-
891-
for (KeyValue keyValue : BucketsStrategy.iterableKeyValues(Layouts.HASH.getFirstInSequence(hash))) {
892-
setNode.executeSet(merged, keyValue.getKey(), keyValue.getValue(), compareByIdentity);
893-
}
894-
895-
final Object[] otherStore = (Object[]) Layouts.HASH.getStore(other);
896-
final int otherSize = Layouts.HASH.getSize(other);
897-
898-
for (int n = 0; n < getContext().getOptions().HASH_PACKED_ARRAY_MAX; n++) {
899-
if (n < otherSize) {
900-
setNode.executeSet(merged, PackedArrayStrategy.getKey(otherStore, n), PackedArrayStrategy.getValue(otherStore, n), compareByIdentity);
901-
}
902-
}
903-
904-
assert HashOperations.verifyStore(getContext(), hash);
905-
return merged;
906-
}
907-
908-
private DynamicObject newHash(DynamicObject hash, Object[] store, int size, boolean compareByIdentity) {
909-
return allocateObjectNode.allocate(Layouts.BASIC_OBJECT.getLogicalClass(hash),
910-
Layouts.HASH.build(store, size, null, null, Layouts.HASH.getDefaultBlock(hash), Layouts.HASH.getDefaultValue(hash), compareByIdentity));
911-
}
912-
913-
protected boolean equalKeys(boolean compareByIdentity, Object key, int hashed, Object otherKey, int otherHashed) {
914-
return compareHashKeysNode.equalKeys(compareByIdentity, key, hashed, otherKey, otherHashed);
915-
}
916-
917-
}
918-
919691
@Primitive(name = "hash_set_default_proc")
920692
public abstract static class SetDefaultProcNode extends PrimitiveArrayArgumentsNode {
921693

‎src/main/ruby/core/truffle/hash_operations.rb

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,11 @@
1111
module Truffle
1212
module HashOperations
1313
def self.hash_merge(current, other)
14-
Truffle.invoke_primitive(:hash_merge, current, other)
14+
new_hash = current.dup
15+
other.each_pair do |k, v|
16+
new_hash[k] = v
17+
end
18+
new_hash
1519
end
1620
end
1721
end

0 commit comments

Comments
 (0)