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

[ur] optimize align allocation in DisjointPool #625

Merged
merged 2 commits into from
Jun 22, 2023

Conversation

igchor
Copy link
Member

@igchor igchor commented Jun 15, 2023

Do not increase size by Alignment - 1 if the requested Alignment is smaller or equal to coarse-grain allocation alignemnt - in such case, Bucket for the allocation will already be properly aligned.

// so the bucket will be properly aligned.
AlignedSize = (Size > 1) ? AlignUp(Size, Alignment) : Alignment;
} else {
AlignedSize = Size + Alignment - 1;
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 not exactly sure what's the purpose of adding alignment to the size here? Can't we just always AlignUp? This should always be called with size being a multiple of alignment anyway (which is what AlignUp enforces).

Copy link
Member Author

Choose a reason for hiding this comment

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

Consider a case where size = alignment = 1MB. Slabs are only aligned to ProviderMinPageSize (let's say 4KB) so aligning up to Alignment is not enough (and we're allocating from Slabs up to 2GB allocations).

When we're allocating directly from the memory provider (line 775) we don't need to do any adjustments, we just leave it to the provider.

Copy link
Contributor

@pbalcer pbalcer Jun 21, 2023

Choose a reason for hiding this comment

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

This is potentially very wasteful if there are many such allocations. We just need to create a bucket that offsets the beginning of the slab. Or we can just create an aligned slab, which should be doable if alignment is page-aligned. And even if it's not, at worst we'd waste 4kb per slab.
Let's leave it as-is for now, but might be worth investigating whether this is a common case and worth optimizing.

But please add a comment here explaining this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Just one clarification - the 2GB I mentioned is actually just the upper limit for poolableSize. By default poolableSize is around a few MB (so allocation bigger than a few MBs will bypass the pooling).

Do not increase size by Alignment - 1 if the requested
Alignment is smaller or equal to coarse-grain allocation
alignemnt - in such case, Bucket for the allocation will
already be properly aligned.
@igchor igchor force-pushed the better_alignment_calculation branch from 9c07701 to 64b944f Compare June 21, 2023 23:28
@pbalcer pbalcer merged commit 7f62c15 into oneapi-src:main Jun 22, 2023
@igchor igchor deleted the better_alignment_calculation branch June 22, 2023 14:46
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