Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fall back to CpuId if failed to get cache size from OS #24989

Merged
3 commits merged into from Jun 11, 2019
Merged

Fall back to CpuId if failed to get cache size from OS #24989

3 commits merged into from Jun 11, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jun 6, 2019

It's possible for GetLogicalProcessorCacheSizeFromOS() to fail;
this happens on alpine linux where it compiles to just return 0;.

As a fallback, we can get the cache size from CpuId. Previously that
was specific to x86; this PR preserves the behavior that we never call
GetLogicalProcessorCacheSizeFromOS on x86.

CpuId only works on x86 and amd64; on other systems we may still return
0 from here. Then GC defaults to a cache size of only 0.25MB.

Fix #16071

It's possible for GetLogicalProcessorCacheSizeFromOS() to fail;
this happens on alpine linux where it compiles to just `return 0;`.

As a fallback, we can get the cache size from CpuId. Previously that
was specific to x86; this PR preserves the behavior that we never call
GetLogicalProcessorCacheSizeFromOS on x86.

CpuId only works on x86 and amd64; on other systems we may still return
0 from here. Then GC defaults to a cache size of only 0.25MB.

Note: Removed the code in an `#ifdef _WIN64` that was nested inside of
`#if defined (_TARGET_X86_)`. Presuming that is dead code.
}
#endif // _TARGET_X86_

// fix this if/when AMD does multicore or SMT
Copy link
Member

@janvorli janvorli Jun 6, 2019

Choose a reason for hiding this comment

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

This comment is very obsolete. I was just about to ask about AMD support. I've found the following CPUID specification from AMD that contains all the necessary info here: https://www.amd.com/system/files/TechDocs/25481.pdf

Edit: Please ignore this comment, I've missed the code above that already handles the AMD.


if (maxSize)
PAL_TRY(Param *, pParam, &param)
Copy link
Member

Choose a reason for hiding this comment

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

Since the attached PAL_EXCEPT_FILTER is using DefaultCatchFilter, the parameter type has to be derived from DefaultCatchFilterParam. The DefaultCatchFilter casts the parameter * to DefaultCatchFilterParam* and reads the pv field out of it.
The DefaultCatchFilter is used to swallow hardware exceptions that can stem from the CPUID.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@ghost ghost requested a review from Maoni0 June 11, 2019 22:34
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!

@ghost ghost merged commit 6d29903 into dotnet:master Jun 11, 2019
@ghost ghost deleted the alpine_cache_size branch June 11, 2019 23:20
@ghost ghost mentioned this pull request Jul 19, 2019
jkotas pushed a commit that referenced this pull request Jul 19, 2019
This typo was in #24989 so would be a new regression in 3.0.
In an x86 build, it causes us to not get the cache size correct,
leading us to use a smaller default cache size and do more GCs.

Tested with GCPerfSim and this PR reduces TotalNumberGCs by 33% using an x86 build.
jkotas pushed a commit to jkotas/coreclr that referenced this pull request Jul 19, 2019
This typo was in dotnet#24989 so would be a new regression in 3.0.
In an x86 build, it causes us to not get the cache size correct,
leading us to use a smaller default cache size and do more GCs.

Tested with GCPerfSim and this PR reduces TotalNumberGCs by 33% using an x86 build.
jkotas pushed a commit to jkotas/coreclr that referenced this pull request Jul 19, 2019
This typo was in dotnet#24989 so would be a new regression in 3.0.
In an x86 build, it causes us to not get the cache size correct,
leading us to use a smaller default cache size and do more GCs.

Tested with GCPerfSim and this PR reduces TotalNumberGCs by 33% using an x86 build.
jkotas pushed a commit that referenced this pull request Jul 19, 2019
This typo was in #24989 so would be a new regression in 3.0.
In an x86 build, it causes us to not get the cache size correct,
leading us to use a smaller default cache size and do more GCs.

Tested with GCPerfSim and this PR reduces TotalNumberGCs by 33% using an x86 build.
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…r#24989)

* Fall back to CpuId if failed to get cache size from OS

It's possible for GetLogicalProcessorCacheSizeFromOS() to fail;
this happens on alpine linux where it compiles to just `return 0;`.

As a fallback, we can get the cache size from CpuId. Previously that
was specific to x86; this PR preserves the behavior that we never call
GetLogicalProcessorCacheSizeFromOS on x86.

CpuId only works on x86 and amd64; on other systems we may still return
0 from here. Then GC defaults to a cache size of only 0.25MB.

Note: Removed the code in an `#ifdef _WIN64` that was nested inside of
`#if defined (_TARGET_X86_)`. Presuming that is dead code.

* Fix exception handler


Commit migrated from dotnet/coreclr@6d29903
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
This typo was in dotnet/coreclr#24989 so would be a new regression in 3.0.
In an x86 build, it causes us to not get the cache size correct,
leading us to use a smaller default cache size and do more GCs.

Tested with GCPerfSim and this PR reduces TotalNumberGCs by 33% using an x86 build.

Commit migrated from dotnet/coreclr@ba39a15
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use GetLogicalProcessorInformationEx in GetLargestOnDieCacheSize
2 participants