Skip to content

[SYCL][CUDA] Cleanup of SYCL context on native unit tests #1824

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 6, 2020
Merged

[SYCL][CUDA] Cleanup of SYCL context on native unit tests #1824

merged 2 commits into from
Jun 6, 2020

Conversation

Ruyk
Copy link
Contributor

@Ruyk Ruyk commented Jun 5, 2020

Fixes #1816

Signed-off-by: Ruyman Reyes ruyman@codeplay.com

@Ruyk
[SYCL][CUDA] Cleanup of SYCL context on native unit tests
8bc1f4a 
Fixes #1816

Signed-off-by: Ruyman Reyes <ruyman@codeplay.com>
@Ruyk Ruyk requested a review from a team as a code owner June 5, 2020 19:32
@Ruyk Ruyk requested a review from rbegam June 5, 2020 19:32
@Ruyk Ruyk mentioned this pull request Jun 5, 2020
Closed
againull
againull reviewed Jun 5, 2020
View reviewed changes
Copy link
Contributor

@againull againull left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for quick fix, checked on my system, this patch indeed resolves #1816.

I am trying to understand what has changed, what was the problem. Could you please explain shortly?

sycl/unittests/pi/cuda/test_interop_get_native.cpp Outdated
}

void TearDown() override {}
void TearDown() override { syclQueue_.release(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks unnecessary to me, this will cause memory leak (we just release ownership, but object will be never destroyed).

@againull againull mentioned this pull request Jun 5, 2020
Merged
@againull againull self-requested a review June 6, 2020 07:30
@againull againull merged commit 06e066a into intel:sycl Jun 6, 2020
@Ruyk
Copy link
Contributor Author

Ruyk commented Jun 6, 2020

Thank you for quick fix, checked on my system, this patch indeed resolves #1816.

I am trying to understand what has changed, what was the problem. Could you please explain shortly?

This has been now merged, but I'll comment briefly:

Patch #1742 switched to using non-primary CUDA context by default. This unit test then creates a number of CUDA contexts in addition to the primary one, which, depending on the platform, may hit the limit on the system (It did not fail initially). By removing the unnecessary additional context variable, and making sure resources are deallocated on test tear down, the problem disappears.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@againull againull againull approved these changes

@rbegam rbegam Awaiting requested review from rbegam

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SYCL][CUDA] Regression on InteropGetNative unit test
2 participants
@Ruyk @againull