-
Notifications
You must be signed in to change notification settings - Fork 44
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
Fix Copy of C Array to Dash Array across multiple units #347
Changes from 2 commits
31364e3
8febae8
9ebbc35
5ac38bd
9a45554
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
--- | ||
Language: Cpp | ||
# BasedOnStyle: Google | ||
AccessModifierOffset: -1 | ||
AlignAfterOpenBracket: Align | ||
AlignConsecutiveAssignments: false | ||
AlignConsecutiveDeclarations: false | ||
AlignEscapedNewlinesLeft: true | ||
AlignOperands: true | ||
AlignTrailingComments: true | ||
AllowAllParametersOfDeclarationOnNextLine: true | ||
AllowShortBlocksOnASingleLine: false | ||
AllowShortCaseLabelsOnASingleLine: false | ||
AllowShortFunctionsOnASingleLine: All | ||
AllowShortIfStatementsOnASingleLine: true | ||
AllowShortLoopsOnASingleLine: true | ||
AlwaysBreakAfterDefinitionReturnType: None | ||
AlwaysBreakAfterReturnType: None | ||
AlwaysBreakBeforeMultilineStrings: true | ||
AlwaysBreakTemplateDeclarations: true | ||
BinPackArguments: true | ||
BinPackParameters: true | ||
BraceWrapping: | ||
AfterClass: false | ||
AfterControlStatement: false | ||
AfterEnum: false | ||
AfterFunction: false | ||
AfterNamespace: false | ||
AfterObjCDeclaration: false | ||
AfterStruct: false | ||
AfterUnion: false | ||
BeforeCatch: false | ||
BeforeElse: false | ||
IndentBraces: false | ||
BreakBeforeBinaryOperators: None | ||
BreakBeforeBraces: Stroustrup | ||
BreakBeforeTernaryOperators: true | ||
#BreakConstructorInitializersBeforeComma: false | ||
BreakConstructorInitializersBeforeComma: true | ||
BreakAfterJavaFieldAnnotations: false | ||
BreakStringLiterals: true | ||
ColumnLimit: 80 | ||
CommentPragmas: '^ IWYU pragma:' | ||
ConstructorInitializerAllOnOneLineOrOnePerLine: false | ||
ConstructorInitializerIndentWidth: 2 | ||
ContinuationIndentWidth: 4 | ||
Cpp11BracedListStyle: true | ||
DerivePointerAlignment: true | ||
DisableFormat: false | ||
ExperimentalAutoDetectBinPacking: false | ||
ForEachMacros: [ foreach, Q_FOREACH, BOOST_FOREACH ] | ||
IncludeCategories: | ||
- Regex: '^<.*\.h>' | ||
Priority: 1 | ||
- Regex: '^<.*' | ||
Priority: 2 | ||
- Regex: '.*' | ||
Priority: 3 | ||
IncludeIsMainRegex: '([-_](test|unittest))?$' | ||
IndentCaseLabels: true | ||
IndentWidth: 2 | ||
IndentWrappedFunctionNames: false | ||
JavaScriptQuotes: Leave | ||
JavaScriptWrapImports: true | ||
KeepEmptyLinesAtTheStartOfBlocks: false | ||
MacroBlockBegin: '' | ||
MacroBlockEnd: '' | ||
MaxEmptyLinesToKeep: 1 | ||
NamespaceIndentation: None | ||
ObjCBlockIndentWidth: 2 | ||
ObjCSpaceAfterProperty: false | ||
ObjCSpaceBeforeProtocolList: false | ||
PenaltyBreakBeforeFirstCallParameter: 1 | ||
PenaltyBreakComment: 300 | ||
PenaltyBreakFirstLessLess: 120 | ||
PenaltyBreakString: 1000 | ||
PenaltyExcessCharacter: 1000000 | ||
PenaltyReturnTypeOnItsOwnLine: 200 | ||
PointerAlignment: Left | ||
ReflowComments: true | ||
SortIncludes: true | ||
SpaceAfterCStyleCast: false | ||
SpaceBeforeAssignmentOperators: true | ||
SpaceBeforeParens: ControlStatements | ||
SpaceInEmptyParentheses: false | ||
SpacesBeforeTrailingComments: 2 | ||
SpacesInAngles: false | ||
SpacesInContainerLiterals: true | ||
SpacesInCStyleCastParentheses: false | ||
SpacesInParentheses: false | ||
SpacesInSquareBrackets: false | ||
#Standard: Auto | ||
Standard: Cpp11 | ||
TabWidth: 8 | ||
UseTab: Never | ||
... | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -493,17 +493,42 @@ GlobOutputIt copy_impl( | |
"l_in_last:", in_last, | ||
"g_out_first:", out_first.pos()); | ||
|
||
|
||
|
||
auto num_elements = std::distance(in_first, in_last); | ||
dart_storage_t ds = dash::dart_storage<ValueType>(num_elements); | ||
DASH_ASSERT_RETURNS( | ||
dart_put_blocking( | ||
out_first.dart_gptr(), | ||
in_first, | ||
ds.nelem, | ||
ds.dtype), | ||
DART_OK); | ||
|
||
auto out_last = out_first + num_elements; | ||
if (num_elements < 1) return out_first; | ||
|
||
auto nremaining = static_cast<dash::default_size_t>(num_elements); | ||
auto pattern = out_first.pattern(); | ||
|
||
while (nremaining) { | ||
// global index to local unit and index | ||
auto local_pos = pattern.local(out_first.pos()); | ||
// number of elements in unit | ||
auto local_size = pattern.local_extents(team_unit_t{local_pos.unit}); | ||
|
||
auto ncopy = std::min( | ||
std::initializer_list<dash::default_size_t>{local_size[0], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above. |
||
nremaining}); | ||
|
||
dart_storage_t ds = dash::dart_storage<ValueType>(ncopy); | ||
|
||
DASH_ASSERT_RETURNS( | ||
dart_put_blocking( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course you are right, thanks. I will do this. Did not think too much about this, I simply extended the already existing code. |
||
out_first.dart_gptr(), | ||
in_first, | ||
ds.nelem, | ||
ds.dtype), | ||
DART_OK); | ||
|
||
std::advance(in_first, ncopy); | ||
std::advance(out_first, ncopy); | ||
|
||
nremaining = std::distance(in_first, in_last); | ||
} | ||
|
||
auto out_last = out_first; | ||
DASH_LOG_TRACE("dash::copy_impl >", | ||
"g_out_last:", out_last.dart_gptr()); | ||
|
||
|
@@ -867,7 +892,7 @@ ValueType * copy( | |
auto total_copy_elem = in_last - in_first; | ||
|
||
// Instead of testing in_first.local() and in_last.local(), this test for | ||
// a local-only range only requires one call to in_first.local() which | ||
// a local-only range only requires one call to in_first.local() which | ||
// increases throughput by ~10% for local ranges. | ||
if (num_local_elem == total_copy_elem) { | ||
// Entire input range is local: | ||
|
@@ -1113,10 +1138,11 @@ GlobOutputIt copy( | |
// Copy to remote elements succeeding the local subrange: | ||
if (g_l_offset_end < out_h_last.pos()) { | ||
DASH_LOG_TRACE("dash::copy", "copy to global succeeding local subrange"); | ||
|
||
out_last = dash::internal::copy_impl( | ||
in_first + l_elem_offset + num_local_elem, | ||
in_last, | ||
out_first + num_local_elem); | ||
in_first + l_elem_offset + num_local_elem, | ||
in_last, | ||
out_first + num_local_elem); | ||
} | ||
} else { | ||
// All elements in output range are remote | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dangerous!
dash::default_size_t
may be 32 bit wide whilenum_elements
is of typesize_t
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::distance
cannot returnsize_t
because it may be negative. And then I check explicitly for a non-negative number.Apart from that I would expect that
dash::default_size_t
is the same assize_t
if available on the underlying platform. Is there any argument against this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it is
ssize_t
. The case I had in mind is the following: the user might decides to compile with the flagDASH_ENABLE_DEFAULT_INDEX_TYPE_INT
(for whatever reason) and then copy more than 4GB into the array, which causesnum_elements
to be truncated intonremaining
. He might even passsize_t
as template parameterIndexType
to the global data structure so it's a valid operation. I think using the pattern's index-type is the safest bet. But that is certainly a highly pathologic case :)