Revert smallIdx optimization for DF indexing (#1833)

Previously, an optimization for the DF _get_head_tail_server function was made that now has less value, since Elliot has optimized some of the overhead that was being avoided there. This code also introduced a bug, so this PR reverts the optimization altogether.
Bears-R-Us · Oct 12, 2022 · 12b5055 · 12b5055
1 parent 9c0b365
commit 12b5055
Show file tree

Hide file tree

Showing 4 changed files with 12 additions and 55 deletions.
diff --git a/src/DataFrameIndexingMsg.chpl b/src/DataFrameIndexingMsg.chpl
@@ -119,10 +119,7 @@
                     var code_vals = toSymEntry(gCode, int);
                     var idxCodeName = dfIdxHelper(idx, code_vals, st, col_name, true);
 
-                    // When smallIdx is set to true, some localization work will
-                    // be skipped and a localizing slice will instead be done,
-                    // which can perform better by avoiding those overhead costs.
-                    var repTup = segPdarrayIndex("str", categories_name, idxCodeName, DType.UInt8, st, smallIdx=true);
+                    var repTup = segPdarrayIndex("str", categories_name, idxCodeName, DType.UInt8, st);
 
                     if repTup.msgType == MsgType.ERROR {
                         throw new IllegalArgumentError(repTup.msg);
@@ -132,10 +129,7 @@
                 }
                 when ("Strings") {
                     dfiLogger.debug(getModuleName(),getRoutineName(),getLineNumber(),"Element at %i is Strings. Name: %s".format(i, ele_parts[2]));
-                    // When smallIdx is set to true, some localization work will
-                    // be skipped and a localizing slice will instead be done,
-                    // which can perform better by avoiding those overhead costs.
-                    var repTup = segPdarrayIndex("str", ele_parts[2], msgArgs.getValueOf("idx_name"), DType.UInt8,  st, smallIdx=true);
+                    var repTup = segPdarrayIndex("str", ele_parts[2], msgArgs.getValueOf("idx_name"), DType.UInt8,  st);
 
                     if repTup.msgType == MsgType.ERROR {
                         throw new IllegalArgumentError(repTup.msg);

diff --git a/src/SegmentedArray.chpl b/src/SegmentedArray.chpl
@@ -113,7 +113,7 @@ module SegmentedArray {
         }
 
         /* Gather segments by index. Returns arrays for the segments and values.*/
-        proc this(iv: [?D] ?t, smallIdx=false) throws where t == int || t == uint {
+        proc this(iv: [?D] ?t) throws where t == int || t == uint {
             use ChplConfig;
 
             // Early return for zero-length result
@@ -164,18 +164,7 @@ module SegmentedArray {
                 t1 = getCurrentTime();
             }
             var gatheredVals = makeDistArray(rtn, values.etype);
-            // Multi-locale requires some extra localization work that is not needed
-            // in CHPL_COMM=none. When smallArr is set to true, a lowLevelLocalizingSlice
-            // will take place that can perform better by avoiding the extra localization
-            // work.
-            if CHPL_COMM != 'none' && smallIdx {
-                forall (go, gl, idx) in zip(gatheredOffsets, gatheredLengths, iv) with (var agg = newDstAggregator(values.etype)) {
-                    var slice = new lowLevelLocalizingSlice(values.a, idx:int..#gl);
-                    for i in 0..#gl {
-                        agg.copy(gatheredVals[go+i], slice.ptr[i]);
-                    }
-                }
-            } else if CHPL_COMM != 'none' {
+            if CHPL_COMM != 'none' {
                 // Compute the src index for each byte in gatheredVals
                 /* For performance, we will do this with a scan, so first we need an array
                 with the difference in index between the current and previous byte. For

diff --git a/src/SegmentedMsg.chpl b/src/SegmentedMsg.chpl
@@ -973,7 +973,7 @@ module SegmentedMsg {
   }
 
   proc segPdarrayIndex(objtype: string, objName: string, iname: string, dtype: DType,
-                       st: borrowed SymTab, smallIdx=false): MsgTuple throws {
+                       st: borrowed SymTab): MsgTuple throws {
     var pn = Reflection.getRoutineName();
     var repMsg: string;
 
@@ -994,10 +994,7 @@ module SegmentedMsg {
                 select gIV.dtype {
                     when DType.Int64 {
                         var iv = toSymEntry(gIV, int);
-                        // When smallIdx is set to true, some localization work will
-                        // be skipped and a localizing slice will instead be done,
-                        // which can perform better by avoiding those overhead costs.
-                        var (newSegs, newVals) = strings[iv.a, smallIdx=smallIdx];
+                        var (newSegs, newVals) = strings[iv.a];
                         var newStringsObj = getSegString(newSegs, newVals, st);
                         newStringsName = newStringsObj.name;
                         nBytes = newStringsObj.nBytes;
@@ -1040,10 +1037,7 @@ module SegmentedMsg {
               select gIV.dtype {
                 when DType.Int64 {
                   var iv = toSymEntry(gIV, int);
-                  // When smallIdx is set to true, some localization work will
-                  // be skipped and a localizing slice will instead be done,
-                  // which can perform better by avoiding those overhead costs.
-                  var (newSegs, newVals) = segArr[iv.a, smallIdx=smallIdx];
+                  var (newSegs, newVals) = segArr[iv.a];
                   var newSegArr = getSegArray(newSegs, newVals, st);
                   newSegArr.fillReturnMap(rtnmap, st);
                 }
@@ -1072,10 +1066,7 @@ module SegmentedMsg {
               select gIV.dtype {
                 when DType.Int64 {
                   var iv = toSymEntry(gIV, int);
-                  // When smallIdx is set to true, some localization work will
-                  // be skipped and a localizing slice will instead be done,
-                  // which can perform better by avoiding those overhead costs.
-                  var (newSegs, newVals) = segArr[iv.a, smallIdx=smallIdx];
+                  var (newSegs, newVals) = segArr[iv.a];
                   var newSegArr = getSegArray(newSegs, newVals, st);
                   newSegArr.fillReturnMap(rtnmap, st);
                 }
@@ -1104,10 +1095,7 @@ module SegmentedMsg {
               select gIV.dtype {
                 when DType.Int64 {
                   var iv = toSymEntry(gIV, int);
-                  // When smallIdx is set to true, some localization work will
-                  // be skipped and a localizing slice will instead be done,
-                  // which can perform better by avoiding those overhead costs.
-                  var (newSegs, newVals) = segArr[iv.a, smallIdx=smallIdx];
+                  var (newSegs, newVals) = segArr[iv.a];
                   var newSegArr = getSegArray(newSegs, newVals, st);
                   newSegArr.fillReturnMap(rtnmap, st);
                 }
@@ -1136,10 +1124,7 @@ module SegmentedMsg {
               select gIV.dtype {
                 when DType.Int64 {
                   var iv = toSymEntry(gIV, int);
-                  // When smallIdx is set to true, some localization work will
-                  // be skipped and a localizing slice will instead be done,
-                  // which can perform better by avoiding those overhead costs.
-                  var (newSegs, newVals) = segArr[iv.a, smallIdx=smallIdx];
+                  var (newSegs, newVals) = segArr[iv.a];
                   var newSegArr = getSegArray(newSegs, newVals, st);
                   newSegArr.fillReturnMap(rtnmap, st);
                 }

diff --git a/src/SegmentedString.chpl b/src/SegmentedString.chpl
@@ -204,7 +204,7 @@ module SegmentedString {
 
     /* Gather strings by index. Returns arrays for the segment offsets
        and bytes of the gathered strings.*/
-    proc this(iv: [?D] ?t, smallIdx=false) throws where t == int || t == uint {
+    proc this(iv: [?D] ?t) throws where t == int || t == uint {
       use ChplConfig;
 
       // Early return for zero-length result
@@ -253,18 +253,7 @@ module SegmentedString {
           t1 = getCurrentTime();
       }
       var gatheredVals = makeDistArray(retBytes, uint(8));
-      // Multi-locale requires some extra localization work that is not needed
-      // in CHPL_COMM=none. When smallArr is set to true, a lowLevelLocalizingSlice
-      // will take place that can perform better by avoiding the extra localization
-      // work.
-      if CHPL_COMM != 'none' && smallIdx {
-        forall (go, gl, idx) in zip(gatheredOffsets, gatheredLengths, iv) with (var agg = newDstAggregator(uint(8))) {
-          var slice = new lowLevelLocalizingSlice(values.a, idx:int..#gl);
-          for i in 0..#gl {
-            agg.copy(gatheredVals[go+i], slice.ptr[i]);
-          }
-        }
-      } else if CHPL_COMM != 'none' {
+      if CHPL_COMM != 'none' {
         // Compute the src index for each byte in gatheredVals
         /* For performance, we will do this with a scan, so first we need an array
            with the difference in index between the current and previous byte. For