Skip to content

Commit

Permalink
Avoid a memory leak when sorting domains (chapel-lang#24205)
Browse files Browse the repository at this point in the history
Follow-up to PR chapel-lang#24147.

Fixes a problem where the test
`test/domains/vass/arrays-of-domains.chpl` would leak memory. For some
reason (that I did not dig in to), code like `var x =
shallowCopyInit(src)` calls the usual initCopy when applied to domains.
But, in the sorting code, we need it to not do that, so I added a
`pragma "no copy"` to avoid it. (Note that such variables already need
`pragma "no auto destroy"` in the sorting code).

Additionally, this PR removes testing for shellSortMoveElts (which is
called within other sort code) from the test module SortTypes because
this was making these test much slower, leading to timeouts.

Reviewed by @vasslitvinov - thanks!

- [x] full comm=none testing

Future Work:
* chapel-lang#16431 investigate "move" memory management for domains, which I
worked around here
  • Loading branch information
mppf committed Jan 18, 2024
2 parents 774462d + 015ec49 commit 27d1dcd
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 7 deletions.
4 changes: 3 additions & 1 deletion modules/packages/Sort.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,7 @@ module InsertionSort {

for i in low..high by stride {
pragma "no auto destroy"
pragma "no copy"
var ithVal = ShallowCopy.shallowCopyInit(Data[i]);

var inserted = false;
Expand Down Expand Up @@ -1157,6 +1158,7 @@ module QuickSort {

// Now swap the pivot to a local variable
pragma "no auto destroy"
pragma "no copy"
var piv: eltType = ShallowCopy.shallowCopyInit(Data[lo]); // leaves Data[lo] empty

while true {
Expand Down Expand Up @@ -1475,6 +1477,7 @@ module ShellSort {
for is in hs..end {
// move Data[is] into v
pragma "no auto destroy"
pragma "no copy"
var v = ShallowCopy.shallowCopyInit(Data[is]);
js = is;
while js >= hs && chpl_compare(v,Data[js-h],comparator) < 0 {
Expand Down Expand Up @@ -2054,7 +2057,6 @@ module ShallowCopy {
}
}

// TODO: These shallowCopy functions should handle Block,Cyclic arrays
inline proc shallowCopy(ref A, dst, src, nElts) {

// Ideally this would just be
Expand Down
6 changes: 0 additions & 6 deletions test/library/packages/Sort/correctness/SortTypes.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,6 @@ proc checkSorts(arr, comparator) {
QuickSort.quickSort(c, comparator=comparator);
assert(isSorted(c, comparator));
assert(a.equals(c));

// check shellSortMoveElts
var d = arr;
ShellSort.shellSortMoveElts(d, comparator=comparator);
assert(isSorted(d, comparator));
assert(a.equals(d));
}

proc checkSorts(arr) {
Expand Down

0 comments on commit 27d1dcd

Please sign in to comment.