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

Fix the heuristics for L3 cache size for Arm64 #71029

Merged
merged 13 commits into from
Jun 22, 2022

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented Jun 21, 2022

While L3 cache size is undetermined on Arm64 machines, if we don't get it, use a fixed heuristics based upon the core count.

1 ~ 4   :  4 MB
5 ~ 16  :  8 MB
17 ~ 64 : 16 MB
65+     : 32 MB

This led to removing the obscure heuristics we have currently that just multiplies the cache size by 3.

#if defined(HOST_ARM64)
    // Bigger gen0 size helps arm64 targets
    maxSize = maxTrueSize * 3;
#endif

@kunalspathak
Copy link
Member Author

@Maoni0

@ghost
Copy link

ghost commented Jun 21, 2022

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

While L3 cache size is undetermined on Arm64 machines, if we don't get it, use a fixed heuristics based upon the core count.

1 ~ 4   :  4 MB
5 ~ 16  :  8 MB
17 ~ 64 : 16 MB
65+     : 32 MB
Author: kunalspathak
Assignees: -
Labels:

area-GC-coreclr

Milestone: -


#ifdef _SC_LEVEL1_DCACHE_SIZE
cacheSize = std::max(cacheSize, ( size_t) sysconf(_SC_LEVEL1_DCACHE_SIZE));
size = ( size_t) sysconf(_SC_LEVEL1_DCACHE_SIZE);
Copy link
Member

Choose a reason for hiding this comment

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

Q (also applies to the old src) - what does sysconf return if there's no cache of that level? I presume it returns 0, not -1 (the doc says it returns -1 if there's an error).

@kunalspathak kunalspathak marked this pull request as ready for review June 21, 2022 21:44
@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib , @davidwrighton , @jkotas

Copy link
Member

@Maoni0 Maoni0 left a comment

Choose a reason for hiding this comment

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

LGTM! thanks @kunalspathak!

@@ -876,21 +876,29 @@ bool ReadMemoryValueFromFile(const char* filename, uint64_t* val)
return result;
}

#define UPDATE_CACHE_SIZE_AND_LEVEL(CACHE_LEVEL) if (size > cacheSize) { cacheSize = size; cacheLevel = CACHE_LEVEL; }
Copy link
Member

Choose a reason for hiding this comment

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

One last nit - please feel free to do that later if you want to get this in before today's snap for preview 6.
Looking at the usages of this macro, I have realized it would be great to make the size a parameter of the macro too. From the usage sites, it is not obvious where it gets the size from (I have to read the macro definition to figure it out).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do it in follow-up PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants