Skip to content

[SYCL] Fix memory leak for sycl::device::get_devices #2256

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

Merged
merged 1 commit into from
Aug 5, 2020

Conversation

bso-intel
Copy link
Contributor

Plugins create pi_device objects for root devices, but
it does not call piDeviceRelease and causes memory leaks.
This slows the performance of SYCL apps.

Signed-off-by: Byoungro So byoungro.so@intel.com

Plugins create pi_device objects for root devices, but
it does not call piDeviceRelease and causes memory leaks.
This slows the performance of SYCL apps.

Signed-off-by: Byoungro So <byoungro.so@intel.com>
@bso-intel bso-intel requested a review from a team as a code owner August 4, 2020 20:00
@bso-intel
Copy link
Contributor Author

@bader @vladimirlaz @romanovvlad
Sorry, I had to create a new branch to show only my own changes.
I accommodated all your feedback in the previous PR. #1923

@bso-intel
Copy link
Contributor Author

@bader, @DoyleLi ,
It seems the failures reported in this PR are already failing in the sycl branch.
I tested with the latest head of sycl branch, and it also failed on the group-algorithm/barrier.cpp.
So, I think these failures are not caused by my change.
Could you confirm if this is true?

//
// UNSUPPORTED: windows
//
//==-----memory-consumption.cpp - SYCL memory consumption basic test ------==//
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test is valuable to check overall memory consumption. But I suggest adding more specific test on the change you are doing, such a verification could be done by setting SYCL_PI_TRACE and checking its output(see sycl/test/scheduler/ReleaseResourcesTest.cpp as an example).

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 will make a follow-up patch for this SYCL_PI_TRACE later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK if additional test will be added in the follow up patch.

@pvchupin pvchupin merged commit 39e7773 into intel:sycl Aug 5, 2020
jsji pushed a commit that referenced this pull request Dec 21, 2023
#2256)

This patch introduces a way to use runtime values for structure fields.

Co-authored-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@a6f017a
jsji pushed a commit that referenced this pull request Jan 11, 2024
…t pt.2 (#2296)

This patch introduces a way to use runtime values for array and vector
types. It continues #2256

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@4db768c
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
Avoid potential ambiguity in UrReturnHelper
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.

3 participants