Skip to content

Conversation

@rdeodhar
Copy link
Contributor

This change extends USM memory pooling to allocations beyond 64KB and also uses pooling for buffer allocations.

@rdeodhar rdeodhar marked this pull request as ready for review March 27, 2021 16:38
@rdeodhar rdeodhar requested review from a team and smaslov-intel as code owners March 27, 2021 16:38
@rdeodhar rdeodhar requested a review from smaslov-intel April 1, 2021 21:24
@rdeodhar rdeodhar requested a review from smaslov-intel April 9, 2021 16:01
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

When I opened this PR again, it showed multiple pending comments (apparently I missed to submit them). Please make sure they are resolved/addressed and request re-review.

@rdeodhar
Copy link
Contributor Author

The CutOff set at 2GB is the maximum poolable size supported by this implementation. The default is 1MB and the cutoff can be changed using the env var. However, even with the env var the upper limit cannot be made bigger than CutOff.

@rdeodhar rdeodhar requested a review from smaslov-intel April 14, 2021 21:42
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

Please address remaining comments

Comment on lines 91 to 94
static size_t MaxPoolableSize = 1;
static size_t Capacity = 4;
static size_t MaxPoolSize = 256;
static size_t CurPoolSize = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be moved inside SetLimits class for better encapsulation?

Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

just minor asks for more comments

@smaslov-intel
Copy link
Contributor

/summary:run

Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

LGTM, but please run pre-commit testing

Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

@rdeodhar - doc LGTM. Sorry for delay.

@bader bader merged commit 4cffedd into intel:sycl Apr 21, 2021
@bader
Copy link
Contributor

bader commented Apr 21, 2021

@rdeodhar, this patch broke post-commit build - https://github.com/intel/llvm/runs/2397183440?check_suite_focus=true.
/home/runner/work/llvm/llvm/src/sycl/plugins/level_zero/usm_allocator.cpp:501:29: error: unused parameter 'Ptr' [-Werror,-Wunused-parameter]

Please, fix ASAP.

jsji pushed a commit that referenced this pull request Nov 4, 2025
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.

4 participants