Skip to content

add GetCacheLineSize() op to memory provider and use it to initialize DP min bucket size#1587

Open
bratpiorka wants to merge 2 commits into
oneapi-src:mainfrom
bratpiorka:rrudnick_get_cacheline_size
Open

add GetCacheLineSize() op to memory provider and use it to initialize DP min bucket size#1587
bratpiorka wants to merge 2 commits into
oneapi-src:mainfrom
bratpiorka:rrudnick_get_cacheline_size

Conversation

@bratpiorka

Copy link
Copy Markdown
Contributor

add GetCacheLineSize() op to memory provider and use it to initialize DP min bucket size

@bratpiorka bratpiorka requested a review from a team as a code owner July 9, 2026 09:07
@bratpiorka bratpiorka force-pushed the rrudnick_get_cacheline_size branch from 2dad996 to 9fe2597 Compare July 9, 2026 09:45
Comment thread src/libumf.def Outdated
Comment thread test/common/ze_loopback.def
Comment thread test/common/ze_loopback.map
@bratpiorka bratpiorka force-pushed the rrudnick_get_cacheline_size branch 3 times, most recently from a287a16 to 63b0991 Compare July 9, 2026 12:42

auto expected_bucket_size = [min_bucket_size](size_t i) -> size_t {
// Even indexes: min,2*min,4*min,...
// Odd indexes: 1.5*min,3*min,6*min,...

@spencerpatty spencerpatty Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ok, I have concerns about the 1.5 * min case since I don't see a requirement for cache_line_size alignment on the front of the array, wouldn't 1.5 * min result in the end of a buffer being halfway through a cache line ? and then the subsequent allocation would start there (causing two allocations to share a cache line? )

I suggest one of two options:

  1. actually enforce starting on min_bucket_size alignment (which is now consistent with cache_line_size)
  2. delete 1.5 * min, so you have Even indices: min, 2 * min, 4 * min ... and Odd Indices: (1.5 * min ->2 * min or deleted), 3 * min, 6 * min ...

? (min_bucket_size << (i / 2))
: ((min_bucket_size + min_bucket_size / 2) << (i / 2));
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The implementation of 2 could be enforced using something like

// stays the same or rounds up to the next multiple of pow_2
// where pow_2 should be a power of two
template<typename T, typename std::enable_if<std::is_integral<T>::value>::type* = nullptr>
T round_up_next_multiple(T count, T pow_2_mult)
{
    assert(pow_2_mult != 0 && ((pow_2_mult & pow_2_mult - 1) == 0)); // check pow_2_mult is a power of two
    return (count + pow_2_mult - 1) & ~(pow_2_mult - 1);
}

// enforce length is a multiple of min_bucket_size, which in round about way enables alignment for subsequent allocations to be on min_bucket_size (aka cache_line_size)
expected_bucket_size = round_up_next_multiple<size_t>(expected_bucket_size, min_bucket_size);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

or something like that with a lambda ... or as the bad pattern is only for the case of "i==3", you could try to modify your ternary into something more complicated. I kind of like the use of this "helper" function since it signals the intent very clearly, even if it is five extra OPs ...

size_t cacheLineSize = 0;
umf_result = umfMemoryProviderGetCacheLineSize(provider, &cacheLineSize);
EXPECT_EQ(umf_result, UMF_RESULT_SUCCESS);
EXPECT_GT(cacheLineSize, 0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be equal to the default of 128 for a level zero memory provider?

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