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

Relocate __lazy_ctor_storage to utils header #1769

Merged
merged 5 commits into from
Aug 8, 2024

Conversation

adamfidel
Copy link
Contributor

@adamfidel adamfidel commented Aug 6, 2024

One of the changes to the upcoming scan rework (#1762) is to move the helper union __lazy_ctor_storage out of the reduce header into the main utils header, as it will eventually be used in scan as well.

This PR makes that movement, as well as adds a few helper methods to __lazy_ctor_storage.


This PR is a part of the following sequence of PRs meant to be merged in order:

#1769 Relocate __lazy_ctor_storage to utils header (This PR)
#1770 Use __result_and_scratch_storage within scan kernels
#1762 Add reduce_then_scan algorithm for transform scan family
#1763 Make Copy_if family of APIs use reduce_then_scan algorithm
#1764 Make Partition family of APIs use reduce_then_scan algorithm
#1765 Make Unique family of APIs use reduce_then_scan algorithm

This work is a collaboration between @mmichel11 @adamfidel and @danhoeflinger, and based upon an original prototype by Ted Painter.

_Tp __v;
__lazy_ctor_storage() {}
void
__setup(const _Tp& init)
Copy link
Contributor

Choose a reason for hiding this comment

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

May be better to use universal reference here?
Otherwise, why we are using const reference?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, good point, we should probably take a forwarding reference and forward it along into the ctor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if a union can have a templated member method but it appears that they can, so I changed the formal parameter to a forwarding reference.

@SergeyKopienko
Copy link
Contributor

One comment about __result.__destroy();
In common case we have this call as latest in some functions.
The problem here is that in case of exception throwing before this code we never execute this line and theoretically may have memory leak.

Probably we may have some helper on stack which will execute this code on it's destructor.

@danhoeflinger
Copy link
Contributor

One comment about __result.__destroy(); In common case we have this call as latest in some functions. The problem here is that in case of exception throwing before this code we never execute this line and theoretically may have memory leak.

Probably we may have some helper on stack which will execute this code on it's destructor.

Its a good point, but I'm not sure how to handle it really. Also, I think the issue is more about not destroying a constructed type than a memory leak. For some types with dynamically allocated memory, not destroying could theoretically lead to a memory leak, but not for simple types. For simple types, the memory would be deallocated as the __lazy_ctor_storage object leaves scope.

The problem is we do not know if it has been setup / constructed at the time of the exception. The point of the structure is to allow us to separate the scope from the construction / destruction without extra memory to record this at runtime. We rely upon the structure of the algorithm to provide the knowledge of if it is constructed.

Copy link
Contributor

@julianmi julianmi left a comment

Choose a reason for hiding this comment

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

LGTM

@adamfidel
Copy link
Contributor Author

One comment about __result.__destroy(); In common case we have this call as latest in some functions. The problem here is that in case of exception throwing before this code we never execute this line and theoretically may have memory leak.
Probably we may have some helper on stack which will execute this code on it's destructor.

Its a good point, but I'm not sure how to handle it really. Also, I think the issue is more about not destroying a constructed type than a memory leak. For some types with dynamically allocated memory, not destroying could theoretically lead to a memory leak, but not for simple types. For simple types, the memory would be deallocated as the __lazy_ctor_storage object leaves scope.

The problem is we do not know if it has been setup / constructed at the time of the exception. The point of the structure is to allow us to separate the scope from the construction / destruction without extra memory to record this at runtime. We rely upon the structure of the algorithm to provide the knowledge of if it is constructed.

I agree with @danhoeflinger here. Additionally, the level of exception safety isn't really changing with this PR as we are ultimately not introducing anything new that wasn't in the code before -- just reorganizing a bit.

@SergeyKopienko are you okay with these changes as they are and tackling the case you brought up in a separate issue / PR?

@SergeyKopienko
Copy link
Contributor

One comment about __result.__destroy(); In common case we have this call as latest in some functions. The problem here is that in case of exception throwing before this code we never execute this line and theoretically may have memory leak.
Probably we may have some helper on stack which will execute this code on it's destructor.

Its a good point, but I'm not sure how to handle it really. Also, I think the issue is more about not destroying a constructed type than a memory leak. For some types with dynamically allocated memory, not destroying could theoretically lead to a memory leak, but not for simple types. For simple types, the memory would be deallocated as the __lazy_ctor_storage object leaves scope.
The problem is we do not know if it has been setup / constructed at the time of the exception. The point of the structure is to allow us to separate the scope from the construction / destruction without extra memory to record this at runtime. We rely upon the structure of the algorithm to provide the knowledge of if it is constructed.

I agree with @danhoeflinger here. Additionally, the level of exception safety isn't really changing with this PR as we are ultimately not introducing anything new that wasn't in the code before -- just reorganizing a bit.

@SergeyKopienko are you okay with these changes as they are and tackling the case you brought up in a separate issue / PR?

I understood and completely agree. Thanks for these explanations.

Copy link
Contributor

@SergeyKopienko SergeyKopienko left a comment

Choose a reason for hiding this comment

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

LGTM

@adamfidel adamfidel merged commit 94d12d2 into main Aug 8, 2024
21 checks passed
@adamfidel adamfidel deleted the dev/shared/reduce_then_scan_plumbing branch August 8, 2024 14:02
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