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

core(view): warn about the implications of without initializing #569

Merged

Conversation

romintomasetti
Copy link
Contributor

@romintomasetti romintomasetti commented Aug 27, 2024

This PR describes what Kokkos::WithoutInitialization implies.

@romintomasetti romintomasetti self-assigned this Aug 27, 2024
docs/Makefile Outdated Show resolved Hide resolved
dalg24
dalg24 previously requested changes Aug 27, 2024
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Please do not mix the change of format with editing the actual content in the same PR

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Please split this pull request up. Either add the changes to initialization first or convert the file first. Also adding another flag in doc/Makefile should be a separate pull request.

@romintomasetti romintomasetti force-pushed the kokkos-core-view-without-initializing branch from b5748d0 to aaed3a0 Compare August 28, 2024 15:22
Comment on lines 466 to 467
For instance, if you need the destructor of your :code:`MyObjectWithDestructor` be called for each element of your :cppkokkos:`View<MyObjectWithDestructor*>`,
you **should not use** :cppkokkos:`WithoutInitialization`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For instance, if you need the destructor of your :code:`MyObjectWithDestructor` be called for each element of your :cppkokkos:`View<MyObjectWithDestructor*>`,
you **should not use** :cppkokkos:`WithoutInitialization`.
For instance, if the View's value type is not trivially destructible,
you **should not use** :cppkokkos:`WithoutInitialization` unless you are taking care of calling the destructor manually before the View deallocates its memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better indeed.

Comment on lines 469 to 470
The reason for that is, since `Kokkos` did not initialize the memory, it cannot know onto which elements of the :cppkokkos:`View` calling the destructor
of the :code:`value_type` is valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, rather something like

Suggested change
The reason for that is, since `Kokkos` did not initialize the memory, it cannot know onto which elements of the :cppkokkos:`View` calling the destructor
of the :code:`value_type` is valid.
The mental model is that whenever placement new is used to call the constructor, the destructor also isn't called before the memory is deallocated but it needs to be be called manually.

Or omit it with my previous suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

@romintomasetti romintomasetti force-pushed the kokkos-core-view-without-initializing branch from aaed3a0 to b421355 Compare August 28, 2024 16:21
Copy link
Contributor

@ajpowelsnl ajpowelsnl left a comment

Choose a reason for hiding this comment

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

Good instinct to improve this documentation. Cleaning up the WithoutInitialization section is the more actionable suggestion. My comment about "novice" is one for broader consideration.

docs/source/ProgrammingGuide/View.rst Show resolved Hide resolved
For instance, if the :cppkokkos:`View`'s value type is not trivially destructible,
you **should not use** :cppkokkos:`WithoutInitialization` unless you are taking care of calling the destructor manually before the :cppkokkos:`View` deallocates its memory.

The mental model is that whenever placement new is used to call the constructor, the destructor also isn't called before the memory is deallocated but it needs to be called manually.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: maybe spell out what to do in the case of both trivially and non-trivially destructible data types, maybe even including a couple of simple code-snippet examples. Perhaps also mention a couple of typical use cases where someone would want to use a View WithoutInitialization.

The sentence on line 469 is not clear, and is a little redundant with the immediately preceding statements. It may not be necessary if the previous verbiage were sharpened up, with spelled out use cases and an couple of simple examples.

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

I think this is sufficient. I you don't understand the difference between trivially destructible and not trivially destructible you should not use View of Views - i.e. you are a novice and should have skipped this section.

@crtrott crtrott merged commit 82ef297 into kokkos:main Sep 3, 2024
2 checks passed
@romintomasetti romintomasetti deleted the kokkos-core-view-without-initializing branch September 4, 2024 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants