From b7435213ee98c491aa7a48dfc82f78e8f945fd17 Mon Sep 17 00:00:00 2001 From: Michael Ferguson Date: Fri, 6 Sep 2024 11:37:20 -0400 Subject: [PATCH] Avoid array allocation for moving to equal domain chpl__coerceMove(_array) already has the ability to omit an array copy when the source and destination arrays refer to the same domain. However, when the domains have equal value but are separate instances, chpl__coerceMove would allocate a separate array and then move elements into it from the original array. This leads to unnecessary memory usage in some cases; for example, a recursive function that returns an array and so declares the return type. This commit adds a mechanism for array implementations to provide a way to steal the data buffers from another array which can be used in this scenario. In some ways this is similar to 'doiOptimizedSwap'. Note that 'doiOptimizedSwap' cannot be used here because we want to avoid allocation for one of the arrays. --- Signed-off-by: Michael Ferguson --- modules/internal/ChapelArray.chpl | 28 +++++++++++++++++++----- modules/internal/ChapelDomain.chpl | 26 ++++++++++++++++++++++ modules/internal/DefaultRectangular.chpl | 24 +++++++++++++++++--- 3 files changed, 70 insertions(+), 8 deletions(-) diff --git a/modules/internal/ChapelArray.chpl b/modules/internal/ChapelArray.chpl index 27942389499..db20262461e 100644 --- a/modules/internal/ChapelArray.chpl +++ b/modules/internal/ChapelArray.chpl @@ -3084,11 +3084,29 @@ module ChapelArray { param moveElts = !typeMismatch; - // If the domains point to the same thing (and aren't just identical), - // then we can simply return the RHS array. - // TODO: if the domain types match, could steal data pointers - if moveElts && dom._instance == rhs.domain._instance { - return rhs; + if moveElts { + // If the domains point to the same thing (and aren't just identical), + // then we can simply return the RHS array. + if dom._instance == rhs.domain._instance { + return rhs; + } + // If the domains have the same value but aren't pointing to the same + // thing, we can ask the array implementation to steal the buffer + if Reflection.canResolveMethod(dom._value, + "doiBuildArrayMoving", + rhs._value) { + if dom == rhs.domain && + dom.distribution == rhs.domain.distribution && + !rhs._unowned { + // ask the array implementation to steal the buffer + pragma "no copy" // avoid error about recursion for initCopy + pragma "unsafe" // when eltType is non-nilable + var lhs = dom.buildArrayMoving(rhs._value); + // destroy the husk that we moved from + _do_destroy_arr(rhs._unowned, rhs._instance, deinitElts=false); + return lhs; + } + } } pragma "no copy" // avoid error about recursion for initCopy diff --git a/modules/internal/ChapelDomain.chpl b/modules/internal/ChapelDomain.chpl index 04a0eca0358..2ceb96cdca1 100644 --- a/modules/internal/ChapelDomain.chpl +++ b/modules/internal/ChapelDomain.chpl @@ -1730,6 +1730,7 @@ module ChapelDomain { } // assumes that data is already initialized + // this function is used in Fortran interop pragma "no copy return" @chpldoc.nodoc proc buildArrayWith(type eltType, data:_ddata(eltType), allocSize:int) { @@ -1748,6 +1749,31 @@ module ChapelDomain { return _newArray(x); } + // assumes that the caller has checked: + // * that 'from' and the receiver domain match & have compatible types + // * that the distributions match as well + // * that the 'from array is not unowned + // * that the domain/array implementation supports doiBuildArrayMoving + pragma "no copy return" + @chpldoc.nodoc + proc buildArrayMoving(from) { + var x = _value.doiBuildArrayMoving(from); + pragma "dont disable remote value forwarding" + proc help() { + _value.add_arr(x); + } + help(); + + chpl_incRefCountsForDomainsInArrayEltTypes(x, x.eltType); + + return _newArray(x); + + // note: 'from' will be deinited here, normally leading to + // deleting the array instance. The array implementation needs + // to have set anything stolen to 'nil' in doiBuildArrayMoving + // and then to take no action on it when deleting. + } + /* An instance of this type is a context manager that can be used in manage statements to resize arrays of non-default-initializable diff --git a/modules/internal/DefaultRectangular.chpl b/modules/internal/DefaultRectangular.chpl index 061ae8c29ef..99f3418e2a5 100644 --- a/modules/internal/DefaultRectangular.chpl +++ b/modules/internal/DefaultRectangular.chpl @@ -717,20 +717,33 @@ module DefaultRectangular { } proc dsiBuildArrayWith(type eltType, data:_ddata(eltType), allocSize:int) { - - var allocRange:range(idxType) = (ranges(0).lowBound)..#allocSize; return new unmanaged DefaultRectangularArr(eltType=eltType, rank=rank, idxType=idxType, strides=strides, + dom=_to_unmanaged(this), // consider the elements already inited initElts=false, // but the array should deinit them deinitElts=true, - dom=_to_unmanaged(this), data=data); } + proc doiBuildArrayMoving(from: DefaultRectangularArr(?)) { + var movedData = from.data; + from.data = nil; // so that it won't be freed when 'from' is deleted + return new unmanaged DefaultRectangularArr( + eltType=from.eltType, + rank=rank, + idxType=idxType, + strides=strides, + // consider the elements already inited + initElts=false, + // but the array should deinit them + deinitElts=true, + dom=_to_unmanaged(this), + data=movedData); + } proc dsiLocalSlice(ranges) { halt("all dsiLocalSlice calls on DefaultRectangulars should be handled in ChapelArray.chpl"); @@ -1137,6 +1150,11 @@ module DefaultRectangular { } override proc dsiDestroyArr(deinitElts:bool) { + // give up early if the array's data has already been stolen + if data == nil { + return; + } + if debugDefaultDist { chpl_debug_writeln("*** DR calling dealloc ", eltType:string); }