Update Map to use chpl__hashtable instead of associative domain/array #15869

daviditen · 2020-06-17T21:49:37Z

Replace the associative domain and array used by Map.map with chpl__hashtable.
Update all the methods to use equivalent hashtable operations.

Add 'pragma "unsafe"' to the map init= method to allow it to work with owned
classes.

Make the map.update argument ref or const ref so it can work with owned classes.

Change a variable of type map from const to var in mason. It is being assigned
into a tuple and has an owned class member within the chpl__hashtable.

Replace the associative domain and array used by Map.map with chpl__hashtable. Update all the methods to use equivalent hashtable operations.

In a couple places map used slot numbers from one table in a different table. Fix it to use slot numbers from the correct table. Add 'pragma "unsafe"' to the map init= method to allow it to work with owned classes. Make the map.update argument ref or const ref so it can work with owned classes. Change a variable of type map from const to var in mason. It is being assigned into a tuple and has an owned class member within the chpl__hashtable.

daviditen · 2020-06-17T21:49:58Z

Passed linux64 paratest.

dlongnecke-cray

Don't be alarmed by all the comments, I think this looks great.

Lots of talk from about inserting isCopyableType checks into methods that make copies of all map elements, to prevent copying of i.e. owned.

As for changes, I think we should emit a compiler error in assignment, init=, toArray, when the type is not copyable instead of using pragma unsafe in init=. If it's there for another reason let me know and perhaps we can find a workaround.

I think there are also some where clauses that can be adjusted and some where-claused iterator overloads can be removed.

I think we have a few followup efforts after this:

Adding parallel iterators (? - We'll want to discuss our vision for container iterators first)
Removing / deprecating workarounds added for non-nilable classes now
Add isCopyableType check to free functions and operators (I should do this for set as well)
Updating map.these to yield key/value pairs by const ref (so map.items).

dlongnecke-cray · 2020-06-17T23:03:06Z

modules/standard/Map.chpl

@@ -157,6 +155,7 @@ module Map {
      :arg parSafe: If `true`, this map will use parallel safe operations.
      :type parSafe: bool
    */
+    pragma "unsafe"


I think instead of marking pragma "unsafe" we should emit an error if either other.keyType or other.valType are not copyable using isCopyableType.

Initializing a map from another map of owned would require removing them from other before adding them to this, which isn't a really well defined operation yet.

TLDR: I don't think init= makes sense for owned classes, or assignment, or any operation that makes a copy of container elements in some fashion (keysToArray/valsToArray).

@mppf Would you tend to agree?

Initializing a map from another map of owned would require removing them from other before adding them to this, which isn't a really well defined operation yet.

I guess it's not clear to me why that's not a well defined operation? This function would completely clear out all the elements in a map of owned. Of course we might think that's not good API design but we could implement it.

It would be OK with me personally if we didn't allow init= from maps with key/value types that aren't returning true for isCopyableType(t).

Maybe instead of not well defined I should say "there's no precedent for it in the language". It's not something we've ever done before, and I think it goes against the expected behavior for init=, assignment, etc.

dlongnecke-cray · 2020-06-17T23:13:35Z

modules/standard/Map.chpl

-        if !myKeys.contains(key) then
-          myKeys += key;
-        vals[key] = m.vals[key];
+    proc update(pragma "intent ref maybe const formal"


We should probably have an isCopyableType check here on the valType, at the very least. To prevent weird behaviors with owned.

dlongnecke-cray · 2020-06-17T23:15:06Z

modules/standard/Map.chpl

@@ -249,40 +252,36 @@ module Map {
      :returns: Reference to the value mapped to the given key.
    */
    proc ref this(k: keyType) ref where !isNonNilableClass(valType) {


I think it would be reasonable to change the where clause to isDefaultInitializable now.

dlongnecke-cray · 2020-06-17T23:15:45Z

modules/standard/Map.chpl

@@ -249,40 +252,36 @@ module Map {
      :returns: Reference to the value mapped to the given key.
    */
    proc ref this(k: keyType) ref where !isNonNilableClass(valType) {
-      _enter();
+      _enter(); defer _leave();

      // TODO: optimize this to avoid looking up the slot multiple times


We can finally remove this TODO! 🎉

dlongnecke-cray · 2020-06-17T23:39:07Z

modules/standard/Map.chpl

@@ -538,12 +538,13 @@ module Map {
      :rtype: [] (keyType, valType)
    */
    proc toArray(): [] (keyType, valType) {


I think we should compilerError if the key type or value type is not copyable.

dlongnecke-cray · 2020-06-18T00:03:08Z

modules/standard/Map.chpl

@@ -315,38 +314,36 @@ module Map {
     */
    proc getReference(k: keyType) ref


I think that with chpl__hashtable as the implementation, it should be possible to remove the where clause. More broadly, I think that with the use of chpl__hashtable it should be possible to remove (or at least deprecate) all the workarounds we added for non-nilable classes due to associative arrays.

We can deprecate/remove in a followup effort after discussion, though.

dlongnecke-cray · 2020-06-18T00:05:37Z

modules/standard/Map.chpl

+        if table.isSlotFull(slot) {
+          ref tabEntry = table.table[slot];
+          yield (tabEntry.key, tabEntry.val);
+        }
      }
    }

    pragma "no doc"
    iter items() const ref where isNonNilableClass(valType) {


I think that with chpl__hashtable as the implementation, it should be possible to remove this overload.

dlongnecke-cray · 2020-06-18T00:06:09Z

modules/standard/Map.chpl

-        yield val;
+      for slot in table.allSlots() {
+        if table.isSlotFull(slot) then
+          yield table.table[slot].val;
      }
    }

    pragma "no doc"
    iter values() const where isNonNilableClass(valType) {


I think that with chpl__hashtable as the implementation, it should be possible to remove this overload.

dlongnecke-cray · 2020-06-18T00:08:20Z

modules/standard/Map.chpl

@@ -615,12 +613,12 @@ module Map {
    :rtype: `bool`
  */
  proc ==(const ref a: map(?kt, ?vt, ?ps), const ref b: map(kt, vt, ps)): bool {
-    for key in a {
-      if !b.contains(key) || a.vals[key] != b.vals[key] then
+    for key in a.keys() {


dlongnecke-cray · 2020-06-18T00:11:16Z

modules/standard/Map.chpl

-
-    for k in newMap do newMap[k] = a.vals[k];
+    // TODO: This is a horrible way to do this. Fix it
+    for ak in a.keys() {


Call .contains? :D (ah I see you do that below, just touch this).

Update functions/methods that copy a map to issue a compiler error if the type to be copied is not copyable. Change `!isNonNilableClass(t)` into `isDefaultInitializable(t)`.

dlongnecke-cray

LGTM if testing passes 👍.

Allow the underlying hash table to shrink itself if desired after removing elements from the map.

David Iten added 2 commits June 15, 2020 18:59

Update Map to use chpl__hashtable instead of associative domain/array

4d3db85

Replace the associative domain and array used by Map.map with chpl__hashtable. Update all the methods to use equivalent hashtable operations.

daviditen requested a review from dlongnecke-cray June 17, 2020 21:50

dlongnecke-cray reviewed Jun 18, 2020

View reviewed changes

Make reviewer suggested edits to map

529c0ea

Update functions/methods that copy a map to issue a compiler error if the type to be copied is not copyable. Change `!isNonNilableClass(t)` into `isDefaultInitializable(t)`.

ronawho mentioned this pull request Jun 18, 2020

Slow non-O(1) amortized access times for HashTables/Maps/AssociativeArrays... #15874

Closed

dlongnecke-cray approved these changes Jun 18, 2020

View reviewed changes

Call chpl__hashtable.maybeShrinkAfterRemove after removing from map

b0d3413

Allow the underlying hash table to shrink itself if desired after removing elements from the map.

daviditen merged commit 5060aa8 into chapel-lang:master Jun 23, 2020

daviditen deleted the map-hashtable branch June 23, 2020 17:20

mppf mentioned this pull request Jun 24, 2020

Problem with runtime type and default initialization #15929

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Map to use chpl__hashtable instead of associative domain/array #15869

Update Map to use chpl__hashtable instead of associative domain/array #15869

daviditen commented Jun 17, 2020

daviditen commented Jun 17, 2020

dlongnecke-cray left a comment •

edited

Loading

dlongnecke-cray Jun 17, 2020

mppf Jun 18, 2020

dlongnecke-cray Jun 18, 2020

dlongnecke-cray Jun 17, 2020

dlongnecke-cray Jun 17, 2020

dlongnecke-cray Jun 17, 2020

dlongnecke-cray Jun 17, 2020

dlongnecke-cray Jun 18, 2020

dlongnecke-cray Jun 18, 2020

dlongnecke-cray Jun 18, 2020

dlongnecke-cray Jun 18, 2020

dlongnecke-cray Jun 18, 2020

dlongnecke-cray Jun 18, 2020

dlongnecke-cray left a comment

		@@ -315,38 +314,36 @@ module Map {
		*/
		proc getReference(k: keyType) ref

Update Map to use chpl__hashtable instead of associative domain/array #15869

Update Map to use chpl__hashtable instead of associative domain/array #15869

Conversation

daviditen commented Jun 17, 2020

daviditen commented Jun 17, 2020

dlongnecke-cray left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongnecke-cray left a comment

Choose a reason for hiding this comment

dlongnecke-cray left a comment •

edited

Loading