-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
adding clrgc variant with regions enabled #89129
Conversation
Tagging subscribers to this area: @dotnet/gc Issue Detailswe already ship standalone clrgc.dll with segments. This adds another clrgcr.dll with regions enabled.
|
@@ -104,6 +104,16 @@ list(APPEND GC_SOURCES ${GC_HEADERS}) | |||
|
|||
convert_to_absolute_path(GC_SOURCES ${GC_SOURCES}) | |||
|
|||
# clrgcr is build with standalone+regions | |||
if (CLR_CMAKE_TARGET_ARCH_ARM64 OR CLR_CMAKE_TARGET_ARCH_AMD64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this builds clrgcr.dll (and libclrgcr.so) on 64 bit platforms. We could change naming if required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: for the sake of readability, would it be better to change the name to: clrgc_regions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didnt want to name it as regions since longer term it might be for a different feature. Perhaps something like experimental or something. Maybe rename it clrgce
for experimental ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clrgcexp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that works, will rename.
CI failures look unrelated but will take a closer look. |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
Ci is now green. Please review, we can discuss naming, but anything we come up with would most likely require changing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! One small nit but it's not necessary.
* adding clrgc variant with regions enabled * only build regions for 64 bit platforms * rename to clrgcexp for experimental
we already ship standalone clrgc.dll with segments. This adds another clrgcr.dll with regions enabled.