Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize remote array copies by creating a local copy of the domain when possible (off by default) #24391

Merged
merged 10 commits into from
Jun 12, 2024
Merged
43 changes: 40 additions & 3 deletions modules/internal/ChapelArray.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -3010,11 +3010,48 @@ module ChapelArray {
return b;
}

//
// These control an optimization in which copying an array from one
// locale to another will also copy the array's domain if we believe
// it's safe to do so. This optimization is currently off by
// default because it was completed close to Chapel 2.1 and we
// wanted more time to live with it before having it on by default.
//
config param localizeConstDomains = true,
debugLocalizedConstDomains = false;

pragma "init copy fn"
proc chpl__initCopy(const ref rhs: [], definedConst: bool) {
pragma "no copy"
var lhs = chpl__coerceCopy(rhs.type, rhs, definedConst);
return lhs;
//
// create a local copy of a remote array's domain if...
// - the optimization is enabled
// - we're running using multiple locales
// - the array copy we're declaring is 'const' (implying that
// its domain can't be re-assigned while it's alive) or the
// original domain is 'const' (implying that it can never
// be re-assigned)
// - the domain is on a remote locale
//
const localize = (localizeConstDomains &&
numLocales > 1 &&
(definedConst || rhs.domain.definedConst) &&
rhs.domain._value.locale != here);
Comment on lines +3035 to +3038
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to add compiledForSingleLocale() here as well? This is a param and could avoid a branch when COMM=none. Arguably not important for now and you could probably just log it as future work in #25238

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea. We could even potentially have both param and non-param aspects of this const split apart and to have the conditional be something like:

if paramLocalize && localize {
  …
} else {
  …
}

s.t. if paramLocalize was false, we'd fold the conditional? I like the idea of forking this off to its own PR, primarily to keep working on things that aren't done yet and adding it to the follow-on issue if you're good with that (though of all the things on that issue, I may do it first—possibly even tomorrow if time permits).

if debugLocalizedConstDomains then
writeln("In initCopy(definedConst=", definedConst,
"), domain definedConst: ", rhs.domain.definedConst, "; ",
if localize then "localizing" else "taking normal path");
if localize {
// localize domain for efficiency since it's OK to do so
const lhsDom = rhs.domain;
pragma "no copy"
var lhs: [lhsDom] rhs.eltType = rhs;
return lhs;
} else {
// otherwise, do what we traditionally have done
pragma "no copy"
var lhs = chpl__coerceCopy(rhs.type, rhs, definedConst);
return lhs;
}
}

// TODO: why is the compiler calling chpl__autoCopy on an array at all?
Expand Down
71 changes: 71 additions & 0 deletions test/arrays/localizeConstDom.chpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
writeln("all var");
{
var A: [1..10] real;
writeln("After A");
var B = A;
writeln("After B");
var C: [1..10] real = A;
writeln("After C");
on Locales[numLocales-1] {
var D = A;
writeln("After D");
local {
var sz = D.size;
}
}
}
writeln();

writeln("const A, var others");
{
const A: [1..10] real;
writeln("After A");
var B = A;
writeln("After B");
var C: [1..10] real = A;
writeln("After C");
on Locales[numLocales-1] {
var D = A;
writeln("After D");
local {
var sz = D.size;
}
}
}
writeln();

writeln("var A, const others");
{
var A: [1..10] real;
writeln("After A");
const B = A;
writeln("After B");
const C: [1..10] real = A;
writeln("After C");
on Locales[numLocales-1] {
const D = A;
writeln("After D");
local {
var sz = D.size;
}
}
}
writeln();

writeln("var Dom, var A, const others");
{
var Dom = {1..10};
var A: [Dom] real;
writeln("After A");
const B = A;
writeln("After B");
const C: [1..10] real = A;
writeln("After C");
on Locales[numLocales-1] {
const D = A;
writeln("After D");
local {
var sz = D.size;
}
}
}
31 changes: 31 additions & 0 deletions test/arrays/localizeConstDom.comm-none.good
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
all var
After A
In initCopy(definedConst=false), domain definedConst: true; taking normal path
After B
After C
In initCopy(definedConst=false), domain definedConst: true; taking normal path
After D

const A, var others
After A
In initCopy(definedConst=false), domain definedConst: true; taking normal path
After B
After C
In initCopy(definedConst=false), domain definedConst: true; taking normal path
After D

var A, const others
After A
In initCopy(definedConst=true), domain definedConst: true; taking normal path
After B
After C
In initCopy(definedConst=true), domain definedConst: true; taking normal path
After D

var Dom, var A, const others
After A
In initCopy(definedConst=true), domain definedConst: false; taking normal path
After B
After C
In initCopy(definedConst=true), domain definedConst: false; taking normal path
After D
1 change: 1 addition & 0 deletions test/arrays/localizeConstDom.compopts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-slocalizeConstDomains=true -sdebugLocalizedConstDomains=true
31 changes: 31 additions & 0 deletions test/arrays/localizeConstDom.good
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
all var
After A
In initCopy(definedConst=false), domain definedConst: true; taking normal path
After B
After C
In initCopy(definedConst=false), domain definedConst: true; localizing
After D

const A, var others
After A
In initCopy(definedConst=false), domain definedConst: true; taking normal path
After B
After C
In initCopy(definedConst=false), domain definedConst: true; localizing
After D

var A, const others
After A
In initCopy(definedConst=true), domain definedConst: true; taking normal path
After B
After C
In initCopy(definedConst=true), domain definedConst: true; localizing
After D

var Dom, var A, const others
After A
In initCopy(definedConst=true), domain definedConst: false; taking normal path
After B
After C
In initCopy(definedConst=true), domain definedConst: false; localizing
After D
1 change: 1 addition & 0 deletions test/arrays/localizeConstDom.numlocales
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
2
12 changes: 12 additions & 0 deletions test/distributions/block/checkLocalizeConstBlockDom.chpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
use BlockDist;

const D = {1..10} dmapped new blockDist({1..100});

var A: [D] real;

on Locales[numLocales-1] {
// This should not cause the localization optimization to fire since
// 'A' is distributed
var B = A;
writeln(B);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-slocalizeConstDomains=true -sdebugLocalizedConstDomains=true
5 changes: 5 additions & 0 deletions test/distributions/block/checkLocalizeConstBlockDom.good
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
In initCopy(definedConst=false), domain definedConst: false; taking normal path
In initCopy(definedConst=false), domain definedConst: false; taking normal path
In initCopy(definedConst=false), domain definedConst: true; taking normal path
In initCopy(definedConst=false), domain definedConst: false; taking normal path
0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0 0.0
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{a = -2, next = {a = 2, next = {a = 3, next = nil}}} {a = -1, next = {a = 3, next = {a = 4, next = nil}}} {a = 0, next = {a = 4, next = {a = 5, next = nil}}} {a = 1, next = {a = 5, next = {a = 6, next = nil}}} {a = 2, next = {a = 6, next = {a = 7, next = nil}}}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-slocalizeConstDomains=false
-slocalizeConstDomains=true # localBlock5-localize.good
Loading