Merge pull request #15895 from ronawho/dom-arr-list-to-chpl__hashtable

Switch the domain's list of arrays to a hashtable

[codeveloped with @mppf]

Domains keep track of the arrays declared over them so that when a
domain is resized it can resize all the arrays declared over it.
Previously, we used a simple LinkedList to store the arrays, which had
performance issues because removal from a linked list is O(n). For the
most part we got lucky and removal happened in reverse insertion order
so removal was effectively O(1). This was because we did serial init and
deinit of arrays-of-arrays, but there are still cases where the
insertion or removal was parallel, which killed performance. This
includes creating arrays in a task-intent clause (#11333) and having a
distributed array of arrays (#9414). This significantly improves the
performance of those cases, and does not hurt serial insertion/deletion
performance.

We have long wanted to use a set/hashtable to store a domain's arrays, but
previously we couldn't because using an associative array inside BaseDom and
BaseDist would result in a circular dependency. Now that chpl__hashtable has
been split out we can just use that. This creates a simple set wrapper over
hashtable. We do this instead of using the standard library set to have more
control and to avoid bringing in all of set code to hopefully limit the impact
on compilation speed.

This improves startup/teardown for a single node forall loop using
array-of-arrays task intents. There's a 75x speedup for ri-a2-AoA.chpl,
which is a reproducer #11333. This also improves deinit for a block dist
array-of-arrays by 500x at 512 nodes (improves non-kernel scalability
for isx -- #9414) Additionally, this will allow us to parallelize array
initialization for all types and array deinitialization in a future PR.

Some technical notes:
 - I had to comment out some `key` printing in chpl__hashtble to avoid
   dynamic dispatch trying to stamp out read/writeThis methods for all
   array types. This resulted in trying to print our arrays of sync and
   we don't support writing syncs.
 - Removing an element that isn't in the set is a noop. This is
   required for optimizations/bulkcomm/bharshbarg/arrayViews, but I made
   inserting an existing element an assertion failure.
 - rootLocaleInitialized is now used earlier, so I moved it to
   ChapelBase.

Resolves #9414
Resolves #11333
Resolves Cray/chapel-private#1036
Closes Cray/chapel-private#1063

modules/internal/ChapelBase.chpl

@@ -22,6 +22,11 @@
 //
 
 module ChapelBase {
+
+  pragma "no doc"
+  pragma "locale private"
+  var rootLocaleInitialized: bool = false;
+
   use ChapelStandard;
   private use ChapelEnv, SysCTypes;
 

modules/internal/ChapelDistribution.chpl

@@ -22,14 +22,14 @@ module ChapelDistribution {
 
   private use ChapelArray, ChapelRange;
   public use ChapelLocks; // maybe make private when fields can be private?
-  public use LinkedLists; // maybe make private when fields can be private?
+  public use ChapelHashtable; // maybe make private when fields can be private?
 
   //
   // Abstract distribution class
   //
   pragma "base dist"
   class BaseDist {
-    var _doms: LinkedList(unmanaged BaseDom); // domains declared over this distribution
+    var _doms: chpl__simpleSet(unmanaged BaseDom); // domains declared over this distribution
     var _domsLock: chpl_LocalSpinlock; // lock for concurrent access
     var _free_when_no_doms: bool; // true when original _distribution is destroyed
     var pid:int = nullPid; // privatized ID, if privatization is supported
@@ -102,7 +102,7 @@ module ChapelDistribution {
     inline proc add_dom(x:unmanaged BaseDom) {
       on this {
         _domsLock.lock();
-        _doms.append(x);
+        _doms.add(x);
         _domsLock.unlock();
       }
     }
@@ -148,7 +148,7 @@ module ChapelDistribution {
   //
   pragma "base domain"
   class BaseDom {
-    var _arrs: LinkedList(unmanaged BaseArr); // arrays declared over this domain
+    var _arrs: chpl__simpleSet(unmanaged BaseArr); // arrays declared over this domain
     var _arrs_containing_dom: int; // number of arrays using this domain
                                    // as var A: [D] [1..2] real
                                    // is using {1..2}
@@ -247,7 +247,7 @@ module ChapelDistribution {
         if locking then
           _arrsLock.lock();
         if addToList then
-          _arrs.append(x);
+          _arrs.add(x);
         else
           _arrs_containing_dom += 1;
         if locking then

modules/internal/ChapelHashtable.chpl

@@ -439,7 +439,7 @@ module ChapelHashtable {
           // This shouldn't be possible since we just garbage collected
           // the deleted entries & the table should only ever be half
           // full of non-deleted entries.
-          halt("couldn't add ", key, " -- ", tableNumFullSlots, " / ", tableSize, " taken");
+          halt("couldn't add key -- ", tableNumFullSlots, " / ", tableSize, " taken");
           return (false, -1);
         }
         return (foundSlot, slotNum);
@@ -575,12 +575,11 @@ module ChapelHashtable {
             // find a destination slot
             var (foundSlot, newslot) = _findSlot(oldEntry.key);
             if foundSlot {
-              halt("duplicate element found while resizing for key ",
-                   oldEntry.key);
+              halt("duplicate element found while resizing for key");
             }
             if newslot < 0 {
               halt("couldn't add element during resize - got slot ", newslot,
-                   " for key ", oldEntry.key);
+                   " for key");
             }
 
             // move the key and value from the old entry into the new one
@@ -634,4 +633,55 @@ module ChapelHashtable {
       rehash(newSizeNum, newSize);
     }
   }
+
+
+  // This is a simple set implementation for internal usage. It's a thin
+  // wrapper over chpl__hashtable. It is not parallel safe and is expected to
+  // be called on the node that owns it (so uses will likely wrap with
+  // locks/on-stmts).
+  record chpl__simpleSet {
+    type eltType;
+    var table: chpl__hashtable(eltType, nothing);
+
+    inline proc size {
+      return table.tableNumFullSlots;
+    }
+
+    proc add(elem) {
+      var (isFullSlot, idx) = table.findAvailableSlot(elem);
+      assert(!isFullSlot);
+      table.fillSlot(idx, elem, none);
+    }
+
+    proc remove(elem) {
+      var (hasFoundSlot, idx) = table.findFullSlot(elem);
+      // note that this is a noop if the element isn't in the set
+      if hasFoundSlot {
+        var key: eltType, val: nothing;
+        table.clearSlot(idx, key, val);
+        table.maybeShrinkAfterRemove();
+      }
+    }
+
+    iter these() {
+      for slot in table.allSlots() do
+        if table.isSlotFull(slot) then
+          yield table.table[slot].key;
+    }
+
+    proc writeThis(f) throws {
+      var count = 1;
+      f <~> "{";
+      for e in this {
+        if count <= (size - 1) {
+          count += 1;
+          f <~> e <~> ", ";
+        } else {
+          f <~> e;
+        }
+      }
+      f <~> "}";
+    }
+  }
+
 }

modules/internal/LocaleModelHelpSetup.chpl

@@ -39,10 +39,6 @@ module LocaleModelHelpSetup {
 
   config param debugLocaleModel = false;
 
-  pragma "no doc"
-  pragma "locale private"
-  var rootLocaleInitialized: bool = false;
-
   extern var chpl_nodeID: chpl_nodeID_t;
 
   record chpl_root_locale_accum {

test/classes/diten/breakList.chpl

@@ -1,5 +1,7 @@
 module OuterModule {
+  use LinkedLists;
   module LibGL {
+    use LinkedLists;
     // Hidden in the bowels of some huge but useful Chapel library like LibGL.
     proc LinkedList.append(x: int) {
       halt("I Got you!");

test/classes/diten/breakList.good

@@ -1,7 +1,7 @@
-breakList.chpl:21: In function 'bar':
-breakList.chpl:24: error: multiple overload sets are applicable to this call
-breakList.chpl:4: note: the best-matching candidate is here
-breakList.chpl:2: note: ... defined in this module
+breakList.chpl:23: In function 'bar':
+breakList.chpl:26: error: multiple overload sets are applicable to this call
+breakList.chpl:6: note: the best-matching candidate is here
+breakList.chpl:3: note: ... defined in this module
 $CHPL_HOME/modules/standard/LinkedLists.chpl:nnnn: note: even though the candidate here is also available
 $CHPL_HOME/modules/standard/LinkedLists.chpl:nnnn: note: ... defined in this module
-breakList.chpl:24: note: use --no-overload-sets-checks to disable overload sets errors
+breakList.chpl:26: note: use --no-overload-sets-checks to disable overload sets errors

test/classes/ferguson/generic-inherit-bug1.chpl

@@ -1,4 +1,5 @@
 module Structure {
+  use LinkedLists;
 
   class GrandParent {
     var gp_field: int;

test/classes/generic/queryGenericField.chpl

@@ -1,3 +1,5 @@
+use LinkedLists;
+
 class R {
   var l: LinkedList(?t);
 }

test/classes/generic/queryGenericField.good

@@ -1 +1 @@
-queryGenericField.chpl:2: error: Query expressions are not currently supported in this context
+queryGenericField.chpl:4: error: Query expressions are not currently supported in this context

test/deprecated/length.chpl

@@ -1,4 +1,4 @@
-use IO, Collection;
+use IO, Collection, LinkedLists;
 
 param bp = b"brad";
 var b = b"brad";

test/io/ferguson/json-list2.chpl

@@ -1,4 +1,4 @@
-use IO;
+use IO, LinkedLists;
 
 record MyRecord {
   var numbers:LinkedList(int); // could it be [1..0] int ?

test/library/standard/LinkedLists/deitz/test_reduce1.chpl

@@ -1,3 +1,5 @@
+use LinkedLists;
+
 var s = makeList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
 
 writeln(s);

test/library/standard/LinkedLists/deitz/test_seq_tuple2.chpl

@@ -1,3 +1,5 @@
+use LinkedLists;
+
 var l = makeList((1, 2), (3, 4));
 
 writeln(l);

test/library/standard/LinkedLists/list-scoped-no-use.chpl

@@ -1,6 +1,4 @@
-// no use List;
-// this test is checking that scoped access to a symbol in an
-// internal module works correctly.
+import LinkedLists;
 
 var x = LinkedLists.makeList(1,2,3);
 writeln(x);

test/memory/shannon/printMemAllocs2.chpl

@@ -1,15 +1,15 @@
 use Memory;
 
 class C {
-  var a: [1..32] int(64);
+  var a: [1..320] int(64);
 }
 
 var c = new unmanaged C();
 var d = new unmanaged C();
 var e = new unmanaged C();
 var f = new unmanaged C();
 
-printMemAllocs(240);
+printMemAllocs(2400);
 
 delete c;
 delete d;

test/memory/shannon/printMemAllocs2.good

@@ -1,9 +1,9 @@
 =================================================================================================================
 Allocated Memory (Bytes)         Number   Size     Total    Description                      Address             
 =================================================================================================================
-printMemAllocs2.chpl:4           32       8        256      array elements                   0xnnnnnnnn  
-printMemAllocs2.chpl:4           32       8        256      array elements                   0xnnnnnnnn  
-printMemAllocs2.chpl:4           32       8        256      array elements                   0xnnnnnnnn  
-printMemAllocs2.chpl:4           32       8        256      array elements                   0xnnnnnnnn  
+printMemAllocs2.chpl:4           320      8        2560     array elements                   0xnnnnnnnn  
+printMemAllocs2.chpl:4           320      8        2560     array elements                   0xnnnnnnnn  
+printMemAllocs2.chpl:4           320      8        2560     array elements                   0xnnnnnnnn  
+printMemAllocs2.chpl:4           320      8        2560     array elements                   0xnnnnnnnn  
 =================================================================================================================
 

test/modules/bradc/printModStuff/foo.good

@@ -34,7 +34,6 @@ Parsing module files:
   $CHPL_HOME/modules/standard/SysError.chpl
   $CHPL_HOME/modules/packages/RangeChunk.chpl
   $CHPL_HOME/modules/packages/Sort.chpl
-  $CHPL_HOME/modules/standard/LinkedLists.chpl
   $CHPL_HOME/modules/packages/Search.chpl
   $CHPL_HOME/modules/standard/Sys.chpl
   $CHPL_HOME/modules/standard/Regexp.chpl

test/modules/sungeun/init/printModuleInitOrder.good

@@ -30,10 +30,10 @@ Initializing Modules:
            ShallowCopy
            InsertionSort
         ChapelDistribution
-         LinkedLists
+         ChapelHashtable
         ExternalArray
         RangeChunk
-        ChapelHashtable
+        ChapelHashing
        ChapelNumLocales
        Sys
       LocaleModelHelpRuntime

test/modules/sungeun/init/printModuleInitOrder.na-none.good

@@ -29,10 +29,10 @@ Initializing Modules:
            ShallowCopy
            InsertionSort
         ChapelDistribution
-         LinkedLists
+         ChapelHashtable
         ExternalArray
         RangeChunk
-        ChapelHashtable
+        ChapelHashing
        ChapelNumLocales
        Sys
       LocaleModelHelpRuntime

test/optimizations/deadCodeElimination/elliot/countDeadModules.comm-ofi.good

@@ -1 +1 @@
-Removed 27 dead modules.
+Removed 26 dead modules.

test/optimizations/deadCodeElimination/elliot/countDeadModules.comm-ugni.good

@@ -1 +1 @@
-Removed 27 dead modules.
+Removed 26 dead modules.

test/optimizations/deadCodeElimination/elliot/countDeadModules.good

@@ -1 +1 @@
-Removed 27 dead modules.
+Removed 26 dead modules.

test/studies/labelprop/labelprop-tweets.chpl

@@ -56,6 +56,7 @@ use IO;
 use Graph;
 use Random;
 use HashedDist;
+use LinkedLists;
 use BlockDist;
 
 // packing twitter user IDs to numbers

test/types/enum/diten/enumDom.chpl

@@ -1,3 +1,5 @@
+use LinkedLists;
+
 enum color { red, green, blue };
 
 var D: domain(real);