Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Review request: getHash candidate for TypeInfo_AssociativeArray #171

Merged
merged 4 commits into from over 1 year ago

3 participants

H. S. Teoh Andrei Alexandrescu Alex Rønne Petersen
H. S. Teoh

To address AA problems such as issue 6210 and issue 7371, I wrote an override for getHash in TypeInfo_AssociativeArray. I've tested the code and confirmed it fixes issues 6210 and 7371. Basically it iterates over key/value pairs, and computes the hash of the key/value's respective hashes, summing the hashes over all pairs. (See code comments for explanation.)

However, I'm not sure if aaA.d is the best place to put this code. It adds another function to the aaA.d API, which changes D's AA ABI. However, I'm not sure if this function is implementable in object_.d without really dirty hacks such as struct AssociativeArray(K,V).{Slot,Hashtable} (and even with those, I don't know how to instantiate AssociativeArray given only the associated TypeInfo object).

Comments?

H. S. Teoh

Rebased to prevent stagnation.

src/rt/aaA.d
... ...
@@ -796,6 +796,32 @@ BB* _d_assocarrayliteralTX(TypeInfo_AssociativeArray ti, void[] keys, void[] val
796 796
 }
797 797
 
798 798
 
  799
+static TypeInfo_AssociativeArray _aaUnwrapTypeInfo(TypeInfo tiRaw)
  800
+{
  801
+    TypeInfo_AssociativeArray ti;
  802
+    while (true)
  803
+    {
  804
+        if ((ti = cast(TypeInfo_AssociativeArray)tiRaw) !is null)
  805
+            break;
  806
+        else if (auto tiConst = cast(TypeInfo_Const)tiRaw) {
1
Andrei Alexandrescu Owner
andralex added a note July 08, 2012

remove "else"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/rt/aaA.d
((8 lines not shown))
  803
+    {
  804
+        if ((ti = cast(TypeInfo_AssociativeArray)tiRaw) !is null)
  805
+            break;
  806
+        else if (auto tiConst = cast(TypeInfo_Const)tiRaw) {
  807
+            // The member in object_.d and object.di differ. This is to ensure
  808
+            //  the file can be compiled both independently in unittest and
  809
+            //  collectively in generating the library. Fixing object.di
  810
+            //  requires changes to std.format in Phobos, fixing object_.d
  811
+            //  makes Phobos's unittest fail, so this hack is employed here to
  812
+            //  avoid irrelevant changes.
  813
+            static if (is(typeof(&tiConst.base) == TypeInfo*))
  814
+                tiRaw = tiConst.base;
  815
+            else
  816
+                tiRaw = tiConst.next;
  817
+        } else
  818
+            assert(0);  // ???
2
Andrei Alexandrescu Owner
andralex added a note July 08, 2012

Add a note mentioning the bugs if this is ever reached.

H. S. Teoh
quickfur added a note July 16, 2012

Hi Andrei, this code was moved verbatim from __aaEqual so that it can be reused. I don't know the reason for the original assert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/rt/aaA.d
... ...
@@ -903,3 +911,78 @@ int _aaEqual(TypeInfo tiRaw, AA e1, AA e2)
903 911
 
904 912
     return 1;           // equal
905 913
 }
  914
+
  915
+
  916
+/*****************************************
  917
+ * Computes a hash value for the entire AA
  918
+ * Returns:
  919
+ *      Hash value
  920
+ */
  921
+extern (C)
  922
+hash_t _aaGetHash(AA* aa, TypeInfo tiRaw)
  923
+{
  924
+    import rt.util.hash;
  925
+
  926
+    hash_t h = 0;
  927
+
  928
+    if (aa.a)
1
Andrei Alexandrescu Owner
andralex added a note July 08, 2012

This guards a large portion of the code. Better yet mention

if (!aa.a) 
{
    return 0;
}

at the very beginning of the function, after which you get to handle the real case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/rt/aaA.d
((24 lines not shown))
  934
+
  935
+        foreach (e; aa.a.b)
  936
+        {
  937
+            while (e)
  938
+            {
  939
+                auto pkey = cast(void*)(e + 1);
  940
+                auto pvalue = pkey + keysize;
  941
+
  942
+                // Compute a hash for the key/value pair by hashing their
  943
+                // respective hash values.
  944
+                hash_t[2] hpair;
  945
+                hpair[0] = e.hash;
  946
+                hpair[1] = valueti.getHash(pvalue);
  947
+
  948
+                // Combine the hash of the key/value pair with the running hash
  949
+                // value using a commutative operator (+) so that the resulting
1
Andrei Alexandrescu Owner
andralex added a note July 08, 2012

s/commutative/associative/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Andrei Alexandrescu
Owner

@quickfur What's the status of this? Thanks!

Andrei Alexandrescu
Owner

ping?

H. S. Teoh

Rebased & updated per Andrei's comments.

Andrei Alexandrescu
Owner

This now breaks the builds, presumably because of the const changes (some of which I'll undo). Stay tuned and please fix for next week. Thanks!

H. S. Teoh

Are you reverting making toHash const? The changes with const toHash are non-trivial. If you're reverting making toHash const, I'm going to wait until afterwards.

Andrei Alexandrescu
Owner

Honest I'm not sure what the best path is for toHash. @9rnsr ?

Andrei Alexandrescu
Owner

ping @9rnsr and @quickfur. How should we move forward with this?

Andrei Alexandrescu
Owner

reping

H. S. Teoh

Sorry for the late update; this morning I finally figured out a simple way to fix the code to work with const toHash without requiring massive changes. So this has been rebased and is ready to merge now.

H. S. Teoh

Rebased to avoid stagnation

H. S. Teoh

Rebased to avoid stagnation.

Alex Rønne Petersen
Collaborator

Assigning to @andralex

H. S. Teoh

Should I squash it too?

Alex Rønne Petersen
Collaborator

@andralex waiting on you

Andrei Alexandrescu
Owner

worksforme

Andrei Alexandrescu andralex merged commit 0650b6e into from November 18, 2012
Andrei Alexandrescu andralex closed this November 18, 2012
Andrei Alexandrescu
Owner

merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.

Showing 2 changed files with 110 additions and 20 deletions. Show diff stats Hide diff stats

  1. 6  src/object_.d
  2. 124  src/rt/aaA.d
6  src/object_.d
@@ -689,6 +689,11 @@ class TypeInfo_AssociativeArray : TypeInfo
689 689
                     this.value == c.value;
690 690
     }
691 691
 
  692
+    override hash_t getHash(in void* p) nothrow @trusted
  693
+    {
  694
+        return _aaGetHash(cast(void*)p, this);
  695
+    }
  696
+
692 697
     // BUG: need to add the rest of the functions
693 698
 
694 699
     override @property size_t tsize() nothrow pure const
@@ -2100,6 +2105,7 @@ extern (C)
2100 2105
     int _aaApply2(void* aa, size_t keysize, _dg2_t dg);
2101 2106
 
2102 2107
     void* _d_assocarrayliteralT(TypeInfo_AssociativeArray ti, size_t length, ...);
  2108
+    hash_t _aaGetHash(void* aa, const(TypeInfo) tiRaw) nothrow;
2103 2109
 }
2104 2110
 
2105 2111
 struct AssociativeArray(Key, Value)
124  src/rt/aaA.d
@@ -103,7 +103,7 @@ struct AA
103 103
  * in value.
104 104
  */
105 105
 
106  
-size_t aligntsize(size_t tsize)
  106
+size_t aligntsize(size_t tsize) nothrow
107 107
 {
108 108
     version (D_LP64)
109 109
         // Size of key needed to align value on 16 bytes
@@ -802,6 +802,34 @@ BB* _d_assocarrayliteralTX(TypeInfo_AssociativeArray ti, void[] keys, void[] val
802 802
 }
803 803
 
804 804
 
  805
+static TypeInfo_AssociativeArray _aaUnwrapTypeInfo(const(TypeInfo) tiRaw) nothrow
  806
+{
  807
+    const(TypeInfo)* p = &tiRaw;
  808
+    TypeInfo_AssociativeArray ti;
  809
+    while (true)
  810
+    {
  811
+        if ((ti = cast(TypeInfo_AssociativeArray)*p) !is null)
  812
+            break;
  813
+
  814
+        if (auto tiConst = cast(TypeInfo_Const)*p) {
  815
+            // The member in object_.d and object.di differ. This is to ensure
  816
+            //  the file can be compiled both independently in unittest and
  817
+            //  collectively in generating the library. Fixing object.di
  818
+            //  requires changes to std.format in Phobos, fixing object_.d
  819
+            //  makes Phobos's unittest fail, so this hack is employed here to
  820
+            //  avoid irrelevant changes.
  821
+            static if (is(typeof(&tiConst.base) == TypeInfo*))
  822
+                p = &tiConst.base;
  823
+            else
  824
+                p = &tiConst.next;
  825
+        } else
  826
+            assert(0);  // ???
  827
+    }
  828
+
  829
+    return ti;
  830
+}
  831
+
  832
+
805 833
 /***********************************
806 834
  * Compare AA contents for equality.
807 835
  * Returns:
@@ -823,25 +851,7 @@ int _aaEqual(TypeInfo tiRaw, AA e1, AA e2)
823 851
 
824 852
     // Check for Bug 5925. ti_raw could be a TypeInfo_Const, we need to unwrap
825 853
     //   it until reaching a real TypeInfo_AssociativeArray.
826  
-    TypeInfo_AssociativeArray ti;
827  
-    while (true)
828  
-    {
829  
-        if ((ti = cast(TypeInfo_AssociativeArray)tiRaw) !is null)
830  
-            break;
831  
-        else if (auto tiConst = cast(TypeInfo_Const)tiRaw) {
832  
-            // The member in object_.d and object.di differ. This is to ensure
833  
-            //  the file can be compiled both independently in unittest and
834  
-            //  collectively in generating the library. Fixing object.di
835  
-            //  requires changes to std.format in Phobos, fixing object_.d
836  
-            //  makes Phobos's unittest fail, so this hack is employed here to
837  
-            //  avoid irrelevant changes.
838  
-            static if (is(typeof(&tiConst.base) == TypeInfo*))
839  
-                tiRaw = tiConst.base;
840  
-            else
841  
-                tiRaw = tiConst.next;
842  
-        } else
843  
-            assert(0);  // ???
844  
-    }
  854
+    TypeInfo_AssociativeArray ti = _aaUnwrapTypeInfo(tiRaw);
845 855
 
846 856
     /* Algorithm: Visit each key/value pair in e1. If that key doesn't exist
847 857
      * in e2, or if the value in e1 doesn't match the one in e2, the arrays
@@ -909,3 +919,77 @@ int _aaEqual(TypeInfo tiRaw, AA e1, AA e2)
909 919
 
910 920
     return 1;           // equal
911 921
 }
  922
+
  923
+
  924
+/*****************************************
  925
+ * Computes a hash value for the entire AA
  926
+ * Returns:
  927
+ *      Hash value
  928
+ */
  929
+extern (C)
  930
+hash_t _aaGetHash(AA* aa, const(TypeInfo) tiRaw) nothrow
  931
+{
  932
+    import rt.util.hash;
  933
+
  934
+    if (!aa.a)
  935
+    	return 0;
  936
+
  937
+    hash_t h = 0;
  938
+    TypeInfo_AssociativeArray ti = _aaUnwrapTypeInfo(tiRaw);
  939
+    auto keyti = ti.key;
  940
+    auto valueti = ti.next;
  941
+    const keysize = aligntsize(keyti.tsize());
  942
+
  943
+    foreach (e; aa.a.b)
  944
+    {
  945
+	while (e)
  946
+	{
  947
+	    auto pkey = cast(void*)(e + 1);
  948
+	    auto pvalue = pkey + keysize;
  949
+
  950
+	    // Compute a hash for the key/value pair by hashing their
  951
+	    // respective hash values.
  952
+	    hash_t[2] hpair;
  953
+	    hpair[0] = e.hash;
  954
+	    hpair[1] = valueti.getHash(pvalue);
  955
+
  956
+	    // Combine the hash of the key/value pair with the running hash
  957
+	    // value using an associative operator (+) so that the resulting
  958
+	    // hash value is independent of the actual order the pairs are
  959
+	    // stored in (important to ensure equality of hash value for two
  960
+	    // AA's containing identical pairs but with different hashtable
  961
+	    // sizes).
  962
+	    h += hashOf(hpair.ptr, hpair.length * hash_t.sizeof);
  963
+
  964
+	    e = e.next;
  965
+	}
  966
+    }
  967
+
  968
+    return h;
  969
+}
  970
+
  971
+unittest
  972
+{
  973
+    string[int] key1 = [1: "true", 2: "false"];
  974
+    string[int] key2 = [1: "false", 2: "true"];
  975
+
  976
+    // AA lits create a larger hashtable
  977
+    int[string[int]] aa1 = [key1: 100, key2: 200];
  978
+
  979
+    // Ensure consistent hash values are computed for key1
  980
+    assert((key1 in aa1) !is null);
  981
+
  982
+    // Manually assigning to an empty AA creates a smaller hashtable
  983
+    int[string[int]] aa2;
  984
+    aa2[key1] = 100;
  985
+    aa2[key2] = 200;
  986
+
  987
+    assert(aa1 == aa2);
  988
+
  989
+    // Ensure binary-independence of equal hash keys
  990
+    string[int] key2a;
  991
+    key2a[1] = "false";
  992
+    key2a[2] = "true";
  993
+
  994
+    assert(aa1[key2a] == 200);
  995
+}
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.