Added the ability to specify a Spin Count Unit via a GC Configuration by mrsharm · Pull Request #84339 · dotnet/runtime

mrsharm · 2023-04-05T01:15:35Z

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

ghost · 2023-04-05T01:15:45Z

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

Issue Details

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

Author:	mrsharm
Assignees:	-
Labels:	`area-GC-coreclr`
Milestone:	-

mangod9 · 2023-04-05T17:24:29Z

Adding @eduardo-vp. Would be good to try out this config to ensure that setting it to something low <10, gets the performance for the app back to 6.0.5 numbers.

Maoni0 · 2023-04-05T19:20:28Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

mrsharm · 2023-04-05T19:20:42Z

@eduardo-vp, to test this out, you'll need to simply set the environment variable: DOTNET_GCSpinCountUnit to some low value in Hex. For example, I tested this with:

$env:DOTNET_GCSpinCountUnit=9

mrsharm · 2023-04-05T19:53:19Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

To test this out, I used a GCPerfSim scenario and debugged both the case where we set DOTNET_GCSpinCountUnit and when we don't by putting breakpoints in the Initialize and SetYieldProcessorScalingFactor functions.

Without The Environment Variable

In the Initialize function, we skip setting the yp_spin_count_unit to that of the config:

0:000> x coreclr!*yp_spin_count_unit*
00007ffe`3059ebcc coreclr!WKS::yp_spin_count_unit = 0

In the SetYieldProcessorScalingFactor function, we set the yp_spin_count_unit to (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9); i.e., the original logic.

With the Environment Variable Set i.e., DOTNET_GCSpinCountUnit=9

In Initialize, I confirmed:

0:000> x coreclr!WKS::*yp_spin_count*
00007ffe`3c5a5710 coreclr!WKS::yp_spin_count_unit = 9

In SetYieldProcessorScalingFactor, I confirmed, we weren't updating the yp_spin_count_unit as we were bypassing the original logic altogether as the spin_count_unit_config_p was true:

0:006> x coreclr!*gc_heap::spin_count_unit_config_p*
00007ffe`3c5a496c coreclr!WKS::gc_heap::spin_count_unit_config_p = true

Do let me know if any further tests are needed from a functional perspective.

Maoni0 · 2023-04-05T20:46:05Z

ok, the new change would work functionally but the code should be moved to next to where we are doing the normal init in initialize_gc, ie, it should be after this code -

#ifdef MULTIPLE_HEAPS
yp_spin_count_unit = 32 * number_of_heaps;
#else
yp_spin_count_unit = 32 * g_num_processors;
#endif //MULTIPLE_HEAPS

as this is now part of the initialization logic for this variable. there's no reason to separate the 2 pieces of code which means someone would need to look at 2 places instead of 1.

and re your question " if any further tests are needed from a functional perspective." - you should actually let it run and make sure that you are seeing the correct value, for example, if you had dumped the value in SetYieldProcessorScalingFactor you would've seen that it was getting the wrong value.

eduardo-vp · 2023-04-05T21:30:29Z

I ran the customer's program with the 48 concurrent instances and got these numbers

Version	Elapsed time
6.0.5 runtime (currently used by the customer)	1:57.74
Current code with DOTNET_GCSpinCountUnit unset	3:35.68
Current code with DOTNET_GCSpinCountUnit=9	2:18.84
Current code with DOTNET_GCSpinCountUnit=8	2:14.75
Current code with DOTNET_GCSpinCountUnit=7	2:04.55
Current code with DOTNET_GCSpinCountUnit=6	2:00.48
Current code with DOTNET_GCSpinCountUnit=5	1:54.24
Current code with DOTNET_GCSpinCountUnit=4	1:42.51
Current code with DOTNET_GCSpinCountUnit=3	1:40.07
Current code with DOTNET_GCSpinCountUnit=2	1:29.19
Current code with DOTNET_GCSpinCountUnit=1	1:24.38

It seems for this case the optimal is reached with DOTNET_GCSpinCountUnit=1 and it's even faster than the version they're currently using (6.0.5)

cshung

LGTM

mrsharm · 2023-04-06T22:56:39Z

/backport to release/7.0

github-actions · 2023-04-06T22:56:51Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4633715849

github-actions · 2023-04-06T22:58:50Z

@mrsharm backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixed spin count problem
Using index info to reconstruct a base tree...
M	src/coreclr/gc/gc.cpp
M	src/coreclr/gc/gcconfig.h
M	src/coreclr/gc/gcpriv.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/gc/gcpriv.h
CONFLICT (content): Merge conflict in src/coreclr/gc/gcpriv.h
Auto-merging src/coreclr/gc/gcconfig.h
Auto-merging src/coreclr/gc/gc.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fixed spin count problem
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-04-06T22:58:51Z

@mrsharm an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

danmoseley · 2023-04-27T17:05:17Z

+ uint32_t saved_yp_spin_count_unit = yp_spin_count_unit;
+ yp_spin_count_unit = (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9);
+
+ // It's very suspicious if it becomes 0 and also, we don't want to spin too much.


if it's suspicious should it assert (yp_spin_count_unit != 0); again here

Yes, it makes sense to assert once more. Will work on adding an additional protective assert.

mrsharm · 2023-04-05T01:15:35Z

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

ghost · 2023-04-05T01:15:45Z

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

Issue Details

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

Author:	mrsharm
Assignees:	-
Labels:	`area-GC-coreclr`
Milestone:	-

mangod9 · 2023-04-05T17:24:29Z

Adding @eduardo-vp. Would be good to try out this config to ensure that setting it to something low <10, gets the performance for the app back to 6.0.5 numbers.

Maoni0 · 2023-04-05T19:20:28Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

mrsharm · 2023-04-05T19:20:42Z

@eduardo-vp, to test this out, you'll need to simply set the environment variable: DOTNET_GCSpinCountUnit to some low value in Hex. For example, I tested this with:

$env:DOTNET_GCSpinCountUnit=9

mrsharm · 2023-04-05T19:53:19Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

To test this out, I used a GCPerfSim scenario and debugged both the case where we set DOTNET_GCSpinCountUnit and when we don't by putting breakpoints in the Initialize and SetYieldProcessorScalingFactor functions.

Without The Environment Variable

In the Initialize function, we skip setting the yp_spin_count_unit to that of the config:

0:000> x coreclr!*yp_spin_count_unit*
00007ffe`3059ebcc coreclr!WKS::yp_spin_count_unit = 0

In the SetYieldProcessorScalingFactor function, we set the yp_spin_count_unit to (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9); i.e., the original logic.

With the Environment Variable Set i.e., DOTNET_GCSpinCountUnit=9

In Initialize, I confirmed:

0:000> x coreclr!WKS::*yp_spin_count*
00007ffe`3c5a5710 coreclr!WKS::yp_spin_count_unit = 9

In SetYieldProcessorScalingFactor, I confirmed, we weren't updating the yp_spin_count_unit as we were bypassing the original logic altogether as the spin_count_unit_config_p was true:

0:006> x coreclr!*gc_heap::spin_count_unit_config_p*
00007ffe`3c5a496c coreclr!WKS::gc_heap::spin_count_unit_config_p = true

Do let me know if any further tests are needed from a functional perspective.

Maoni0 · 2023-04-05T20:46:05Z

ok, the new change would work functionally but the code should be moved to next to where we are doing the normal init in initialize_gc, ie, it should be after this code -

#ifdef MULTIPLE_HEAPS
yp_spin_count_unit = 32 * number_of_heaps;
#else
yp_spin_count_unit = 32 * g_num_processors;
#endif //MULTIPLE_HEAPS

as this is now part of the initialization logic for this variable. there's no reason to separate the 2 pieces of code which means someone would need to look at 2 places instead of 1.

and re your question " if any further tests are needed from a functional perspective." - you should actually let it run and make sure that you are seeing the correct value, for example, if you had dumped the value in SetYieldProcessorScalingFactor you would've seen that it was getting the wrong value.

eduardo-vp · 2023-04-05T21:30:29Z

I ran the customer's program with the 48 concurrent instances and got these numbers

Version	Elapsed time
6.0.5 runtime (currently used by the customer)	1:57.74
Current code with DOTNET_GCSpinCountUnit unset	3:35.68
Current code with DOTNET_GCSpinCountUnit=9	2:18.84
Current code with DOTNET_GCSpinCountUnit=8	2:14.75
Current code with DOTNET_GCSpinCountUnit=7	2:04.55
Current code with DOTNET_GCSpinCountUnit=6	2:00.48
Current code with DOTNET_GCSpinCountUnit=5	1:54.24
Current code with DOTNET_GCSpinCountUnit=4	1:42.51
Current code with DOTNET_GCSpinCountUnit=3	1:40.07
Current code with DOTNET_GCSpinCountUnit=2	1:29.19
Current code with DOTNET_GCSpinCountUnit=1	1:24.38

It seems for this case the optimal is reached with DOTNET_GCSpinCountUnit=1 and it's even faster than the version they're currently using (6.0.5)

cshung

LGTM

mrsharm · 2023-04-06T22:56:39Z

/backport to release/7.0

github-actions · 2023-04-06T22:56:51Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4633715849

github-actions · 2023-04-06T22:58:50Z

@mrsharm backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixed spin count problem
Using index info to reconstruct a base tree...
M	src/coreclr/gc/gc.cpp
M	src/coreclr/gc/gcconfig.h
M	src/coreclr/gc/gcpriv.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/gc/gcpriv.h
CONFLICT (content): Merge conflict in src/coreclr/gc/gcpriv.h
Auto-merging src/coreclr/gc/gcconfig.h
Auto-merging src/coreclr/gc/gc.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fixed spin count problem
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-04-06T22:58:51Z

@mrsharm an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

danmoseley · 2023-04-27T17:05:17Z

+ uint32_t saved_yp_spin_count_unit = yp_spin_count_unit;
+ yp_spin_count_unit = (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9);
+
+ // It's very suspicious if it becomes 0 and also, we don't want to spin too much.


if it's suspicious should it assert (yp_spin_count_unit != 0); again here

Yes, it makes sense to assert once more. Will work on adding an additional protective assert.

mrsharm · 2023-04-05T01:15:35Z

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

ghost · 2023-04-05T01:15:45Z

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

Issue Details

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

Author:	mrsharm
Assignees:	-
Labels:	`area-GC-coreclr`
Milestone:	-

mangod9 · 2023-04-05T17:24:29Z

Adding @eduardo-vp. Would be good to try out this config to ensure that setting it to something low <10, gets the performance for the app back to 6.0.5 numbers.

Maoni0 · 2023-04-05T19:20:28Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

mrsharm · 2023-04-05T19:20:42Z

@eduardo-vp, to test this out, you'll need to simply set the environment variable: DOTNET_GCSpinCountUnit to some low value in Hex. For example, I tested this with:

$env:DOTNET_GCSpinCountUnit=9

mrsharm · 2023-04-05T19:53:19Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

To test this out, I used a GCPerfSim scenario and debugged both the case where we set DOTNET_GCSpinCountUnit and when we don't by putting breakpoints in the Initialize and SetYieldProcessorScalingFactor functions.

Without The Environment Variable

In the Initialize function, we skip setting the yp_spin_count_unit to that of the config:

0:000> x coreclr!*yp_spin_count_unit*
00007ffe`3059ebcc coreclr!WKS::yp_spin_count_unit = 0

In the SetYieldProcessorScalingFactor function, we set the yp_spin_count_unit to (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9); i.e., the original logic.

With the Environment Variable Set i.e., DOTNET_GCSpinCountUnit=9

In Initialize, I confirmed:

0:000> x coreclr!WKS::*yp_spin_count*
00007ffe`3c5a5710 coreclr!WKS::yp_spin_count_unit = 9

In SetYieldProcessorScalingFactor, I confirmed, we weren't updating the yp_spin_count_unit as we were bypassing the original logic altogether as the spin_count_unit_config_p was true:

0:006> x coreclr!*gc_heap::spin_count_unit_config_p*
00007ffe`3c5a496c coreclr!WKS::gc_heap::spin_count_unit_config_p = true

Do let me know if any further tests are needed from a functional perspective.

Maoni0 · 2023-04-05T20:46:05Z

ok, the new change would work functionally but the code should be moved to next to where we are doing the normal init in initialize_gc, ie, it should be after this code -

#ifdef MULTIPLE_HEAPS
yp_spin_count_unit = 32 * number_of_heaps;
#else
yp_spin_count_unit = 32 * g_num_processors;
#endif //MULTIPLE_HEAPS

as this is now part of the initialization logic for this variable. there's no reason to separate the 2 pieces of code which means someone would need to look at 2 places instead of 1.

and re your question " if any further tests are needed from a functional perspective." - you should actually let it run and make sure that you are seeing the correct value, for example, if you had dumped the value in SetYieldProcessorScalingFactor you would've seen that it was getting the wrong value.

eduardo-vp · 2023-04-05T21:30:29Z

I ran the customer's program with the 48 concurrent instances and got these numbers

Version	Elapsed time
6.0.5 runtime (currently used by the customer)	1:57.74
Current code with DOTNET_GCSpinCountUnit unset	3:35.68
Current code with DOTNET_GCSpinCountUnit=9	2:18.84
Current code with DOTNET_GCSpinCountUnit=8	2:14.75
Current code with DOTNET_GCSpinCountUnit=7	2:04.55
Current code with DOTNET_GCSpinCountUnit=6	2:00.48
Current code with DOTNET_GCSpinCountUnit=5	1:54.24
Current code with DOTNET_GCSpinCountUnit=4	1:42.51
Current code with DOTNET_GCSpinCountUnit=3	1:40.07
Current code with DOTNET_GCSpinCountUnit=2	1:29.19
Current code with DOTNET_GCSpinCountUnit=1	1:24.38

It seems for this case the optimal is reached with DOTNET_GCSpinCountUnit=1 and it's even faster than the version they're currently using (6.0.5)

cshung

LGTM

mrsharm · 2023-04-06T22:56:39Z

/backport to release/7.0

github-actions · 2023-04-06T22:56:51Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4633715849

github-actions · 2023-04-06T22:58:50Z

@mrsharm backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixed spin count problem
Using index info to reconstruct a base tree...
M	src/coreclr/gc/gc.cpp
M	src/coreclr/gc/gcconfig.h
M	src/coreclr/gc/gcpriv.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/gc/gcpriv.h
CONFLICT (content): Merge conflict in src/coreclr/gc/gcpriv.h
Auto-merging src/coreclr/gc/gcconfig.h
Auto-merging src/coreclr/gc/gc.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fixed spin count problem
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-04-06T22:58:51Z

@mrsharm an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

danmoseley · 2023-04-27T17:05:17Z

+ uint32_t saved_yp_spin_count_unit = yp_spin_count_unit;
+ yp_spin_count_unit = (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9);
+
+ // It's very suspicious if it becomes 0 and also, we don't want to spin too much.


if it's suspicious should it assert (yp_spin_count_unit != 0); again here

Yes, it makes sense to assert once more. Will work on adding an additional protective assert.

mrsharm · 2023-04-05T01:15:35Z

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

ghost · 2023-04-05T01:15:45Z

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

Issue Details

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

Author:	mrsharm
Assignees:	-
Labels:	`area-GC-coreclr`
Milestone:	-

mangod9 · 2023-04-05T17:24:29Z

Adding @eduardo-vp. Would be good to try out this config to ensure that setting it to something low <10, gets the performance for the app back to 6.0.5 numbers.

Maoni0 · 2023-04-05T19:20:28Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

mrsharm · 2023-04-05T19:20:42Z

@eduardo-vp, to test this out, you'll need to simply set the environment variable: DOTNET_GCSpinCountUnit to some low value in Hex. For example, I tested this with:

$env:DOTNET_GCSpinCountUnit=9

mrsharm · 2023-04-05T19:53:19Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

To test this out, I used a GCPerfSim scenario and debugged both the case where we set DOTNET_GCSpinCountUnit and when we don't by putting breakpoints in the Initialize and SetYieldProcessorScalingFactor functions.

Without The Environment Variable

In the Initialize function, we skip setting the yp_spin_count_unit to that of the config:

0:000> x coreclr!*yp_spin_count_unit*
00007ffe`3059ebcc coreclr!WKS::yp_spin_count_unit = 0

In the SetYieldProcessorScalingFactor function, we set the yp_spin_count_unit to (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9); i.e., the original logic.

With the Environment Variable Set i.e., DOTNET_GCSpinCountUnit=9

In Initialize, I confirmed:

0:000> x coreclr!WKS::*yp_spin_count*
00007ffe`3c5a5710 coreclr!WKS::yp_spin_count_unit = 9

In SetYieldProcessorScalingFactor, I confirmed, we weren't updating the yp_spin_count_unit as we were bypassing the original logic altogether as the spin_count_unit_config_p was true:

0:006> x coreclr!*gc_heap::spin_count_unit_config_p*
00007ffe`3c5a496c coreclr!WKS::gc_heap::spin_count_unit_config_p = true

Do let me know if any further tests are needed from a functional perspective.

Maoni0 · 2023-04-05T20:46:05Z

ok, the new change would work functionally but the code should be moved to next to where we are doing the normal init in initialize_gc, ie, it should be after this code -

#ifdef MULTIPLE_HEAPS
yp_spin_count_unit = 32 * number_of_heaps;
#else
yp_spin_count_unit = 32 * g_num_processors;
#endif //MULTIPLE_HEAPS

as this is now part of the initialization logic for this variable. there's no reason to separate the 2 pieces of code which means someone would need to look at 2 places instead of 1.

and re your question " if any further tests are needed from a functional perspective." - you should actually let it run and make sure that you are seeing the correct value, for example, if you had dumped the value in SetYieldProcessorScalingFactor you would've seen that it was getting the wrong value.

eduardo-vp · 2023-04-05T21:30:29Z

I ran the customer's program with the 48 concurrent instances and got these numbers

Version	Elapsed time
6.0.5 runtime (currently used by the customer)	1:57.74
Current code with DOTNET_GCSpinCountUnit unset	3:35.68
Current code with DOTNET_GCSpinCountUnit=9	2:18.84
Current code with DOTNET_GCSpinCountUnit=8	2:14.75
Current code with DOTNET_GCSpinCountUnit=7	2:04.55
Current code with DOTNET_GCSpinCountUnit=6	2:00.48
Current code with DOTNET_GCSpinCountUnit=5	1:54.24
Current code with DOTNET_GCSpinCountUnit=4	1:42.51
Current code with DOTNET_GCSpinCountUnit=3	1:40.07
Current code with DOTNET_GCSpinCountUnit=2	1:29.19
Current code with DOTNET_GCSpinCountUnit=1	1:24.38

It seems for this case the optimal is reached with DOTNET_GCSpinCountUnit=1 and it's even faster than the version they're currently using (6.0.5)

cshung

LGTM

mrsharm · 2023-04-06T22:56:39Z

/backport to release/7.0

github-actions · 2023-04-06T22:56:51Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4633715849

github-actions · 2023-04-06T22:58:50Z

@mrsharm backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixed spin count problem
Using index info to reconstruct a base tree...
M	src/coreclr/gc/gc.cpp
M	src/coreclr/gc/gcconfig.h
M	src/coreclr/gc/gcpriv.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/gc/gcpriv.h
CONFLICT (content): Merge conflict in src/coreclr/gc/gcpriv.h
Auto-merging src/coreclr/gc/gcconfig.h
Auto-merging src/coreclr/gc/gc.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fixed spin count problem
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-04-06T22:58:51Z

@mrsharm an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

danmoseley · 2023-04-27T17:05:17Z

+ uint32_t saved_yp_spin_count_unit = yp_spin_count_unit;
+ yp_spin_count_unit = (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9);
+
+ // It's very suspicious if it becomes 0 and also, we don't want to spin too much.


if it's suspicious should it assert (yp_spin_count_unit != 0); again here

Yes, it makes sense to assert once more. Will work on adding an additional protective assert.

mrsharm · 2023-04-05T01:15:35Z

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

ghost · 2023-04-05T01:15:45Z

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

Issue Details

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

Author:	mrsharm
Assignees:	-
Labels:	`area-GC-coreclr`
Milestone:	-

mangod9 · 2023-04-05T17:24:29Z

Adding @eduardo-vp. Would be good to try out this config to ensure that setting it to something low <10, gets the performance for the app back to 6.0.5 numbers.

Maoni0 · 2023-04-05T19:20:28Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

mrsharm · 2023-04-05T19:20:42Z

@eduardo-vp, to test this out, you'll need to simply set the environment variable: DOTNET_GCSpinCountUnit to some low value in Hex. For example, I tested this with:

$env:DOTNET_GCSpinCountUnit=9

mrsharm · 2023-04-05T19:53:19Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

To test this out, I used a GCPerfSim scenario and debugged both the case where we set DOTNET_GCSpinCountUnit and when we don't by putting breakpoints in the Initialize and SetYieldProcessorScalingFactor functions.

Without The Environment Variable

In the Initialize function, we skip setting the yp_spin_count_unit to that of the config:

0:000> x coreclr!*yp_spin_count_unit*
00007ffe`3059ebcc coreclr!WKS::yp_spin_count_unit = 0

In the SetYieldProcessorScalingFactor function, we set the yp_spin_count_unit to (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9); i.e., the original logic.

With the Environment Variable Set i.e., DOTNET_GCSpinCountUnit=9

In Initialize, I confirmed:

0:000> x coreclr!WKS::*yp_spin_count*
00007ffe`3c5a5710 coreclr!WKS::yp_spin_count_unit = 9

In SetYieldProcessorScalingFactor, I confirmed, we weren't updating the yp_spin_count_unit as we were bypassing the original logic altogether as the spin_count_unit_config_p was true:

0:006> x coreclr!*gc_heap::spin_count_unit_config_p*
00007ffe`3c5a496c coreclr!WKS::gc_heap::spin_count_unit_config_p = true

Do let me know if any further tests are needed from a functional perspective.

Maoni0 · 2023-04-05T20:46:05Z

ok, the new change would work functionally but the code should be moved to next to where we are doing the normal init in initialize_gc, ie, it should be after this code -

#ifdef MULTIPLE_HEAPS
yp_spin_count_unit = 32 * number_of_heaps;
#else
yp_spin_count_unit = 32 * g_num_processors;
#endif //MULTIPLE_HEAPS

as this is now part of the initialization logic for this variable. there's no reason to separate the 2 pieces of code which means someone would need to look at 2 places instead of 1.

and re your question " if any further tests are needed from a functional perspective." - you should actually let it run and make sure that you are seeing the correct value, for example, if you had dumped the value in SetYieldProcessorScalingFactor you would've seen that it was getting the wrong value.

eduardo-vp · 2023-04-05T21:30:29Z

I ran the customer's program with the 48 concurrent instances and got these numbers

Version	Elapsed time
6.0.5 runtime (currently used by the customer)	1:57.74
Current code with DOTNET_GCSpinCountUnit unset	3:35.68
Current code with DOTNET_GCSpinCountUnit=9	2:18.84
Current code with DOTNET_GCSpinCountUnit=8	2:14.75
Current code with DOTNET_GCSpinCountUnit=7	2:04.55
Current code with DOTNET_GCSpinCountUnit=6	2:00.48
Current code with DOTNET_GCSpinCountUnit=5	1:54.24
Current code with DOTNET_GCSpinCountUnit=4	1:42.51
Current code with DOTNET_GCSpinCountUnit=3	1:40.07
Current code with DOTNET_GCSpinCountUnit=2	1:29.19
Current code with DOTNET_GCSpinCountUnit=1	1:24.38

It seems for this case the optimal is reached with DOTNET_GCSpinCountUnit=1 and it's even faster than the version they're currently using (6.0.5)

cshung

LGTM

mrsharm · 2023-04-06T22:56:39Z

/backport to release/7.0

github-actions · 2023-04-06T22:56:51Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4633715849

github-actions · 2023-04-06T22:58:50Z

@mrsharm backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixed spin count problem
Using index info to reconstruct a base tree...
M	src/coreclr/gc/gc.cpp
M	src/coreclr/gc/gcconfig.h
M	src/coreclr/gc/gcpriv.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/gc/gcpriv.h
CONFLICT (content): Merge conflict in src/coreclr/gc/gcpriv.h
Auto-merging src/coreclr/gc/gcconfig.h
Auto-merging src/coreclr/gc/gc.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fixed spin count problem
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-04-06T22:58:51Z

@mrsharm an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

danmoseley · 2023-04-27T17:05:17Z

+ uint32_t saved_yp_spin_count_unit = yp_spin_count_unit;
+ yp_spin_count_unit = (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9);
+
+ // It's very suspicious if it becomes 0 and also, we don't want to spin too much.


if it's suspicious should it assert (yp_spin_count_unit != 0); again here

Yes, it makes sense to assert once more. Will work on adding an additional protective assert.

mrsharm · 2023-04-05T01:15:35Z

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

ghost · 2023-04-05T01:15:45Z

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

Issue Details

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

Author:	mrsharm
Assignees:	-
Labels:	`area-GC-coreclr`
Milestone:	-

mangod9 · 2023-04-05T17:24:29Z

Adding @eduardo-vp. Would be good to try out this config to ensure that setting it to something low <10, gets the performance for the app back to 6.0.5 numbers.

Maoni0 · 2023-04-05T19:20:28Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

mrsharm · 2023-04-05T19:20:42Z

@eduardo-vp, to test this out, you'll need to simply set the environment variable: DOTNET_GCSpinCountUnit to some low value in Hex. For example, I tested this with:

$env:DOTNET_GCSpinCountUnit=9

mrsharm · 2023-04-05T19:53:19Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

To test this out, I used a GCPerfSim scenario and debugged both the case where we set DOTNET_GCSpinCountUnit and when we don't by putting breakpoints in the Initialize and SetYieldProcessorScalingFactor functions.

Without The Environment Variable

In the Initialize function, we skip setting the yp_spin_count_unit to that of the config:

0:000> x coreclr!*yp_spin_count_unit*
00007ffe`3059ebcc coreclr!WKS::yp_spin_count_unit = 0

In the SetYieldProcessorScalingFactor function, we set the yp_spin_count_unit to (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9); i.e., the original logic.

With the Environment Variable Set i.e., DOTNET_GCSpinCountUnit=9

In Initialize, I confirmed:

0:000> x coreclr!WKS::*yp_spin_count*
00007ffe`3c5a5710 coreclr!WKS::yp_spin_count_unit = 9

In SetYieldProcessorScalingFactor, I confirmed, we weren't updating the yp_spin_count_unit as we were bypassing the original logic altogether as the spin_count_unit_config_p was true:

0:006> x coreclr!*gc_heap::spin_count_unit_config_p*
00007ffe`3c5a496c coreclr!WKS::gc_heap::spin_count_unit_config_p = true

Do let me know if any further tests are needed from a functional perspective.

Maoni0 · 2023-04-05T20:46:05Z

ok, the new change would work functionally but the code should be moved to next to where we are doing the normal init in initialize_gc, ie, it should be after this code -

#ifdef MULTIPLE_HEAPS
yp_spin_count_unit = 32 * number_of_heaps;
#else
yp_spin_count_unit = 32 * g_num_processors;
#endif //MULTIPLE_HEAPS

as this is now part of the initialization logic for this variable. there's no reason to separate the 2 pieces of code which means someone would need to look at 2 places instead of 1.

and re your question " if any further tests are needed from a functional perspective." - you should actually let it run and make sure that you are seeing the correct value, for example, if you had dumped the value in SetYieldProcessorScalingFactor you would've seen that it was getting the wrong value.

eduardo-vp · 2023-04-05T21:30:29Z

I ran the customer's program with the 48 concurrent instances and got these numbers

Version	Elapsed time
6.0.5 runtime (currently used by the customer)	1:57.74
Current code with DOTNET_GCSpinCountUnit unset	3:35.68
Current code with DOTNET_GCSpinCountUnit=9	2:18.84
Current code with DOTNET_GCSpinCountUnit=8	2:14.75
Current code with DOTNET_GCSpinCountUnit=7	2:04.55
Current code with DOTNET_GCSpinCountUnit=6	2:00.48
Current code with DOTNET_GCSpinCountUnit=5	1:54.24
Current code with DOTNET_GCSpinCountUnit=4	1:42.51
Current code with DOTNET_GCSpinCountUnit=3	1:40.07
Current code with DOTNET_GCSpinCountUnit=2	1:29.19
Current code with DOTNET_GCSpinCountUnit=1	1:24.38

It seems for this case the optimal is reached with DOTNET_GCSpinCountUnit=1 and it's even faster than the version they're currently using (6.0.5)

cshung

LGTM

mrsharm · 2023-04-06T22:56:39Z

/backport to release/7.0

github-actions · 2023-04-06T22:56:51Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4633715849

github-actions · 2023-04-06T22:58:50Z

@mrsharm backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixed spin count problem
Using index info to reconstruct a base tree...
M	src/coreclr/gc/gc.cpp
M	src/coreclr/gc/gcconfig.h
M	src/coreclr/gc/gcpriv.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/gc/gcpriv.h
CONFLICT (content): Merge conflict in src/coreclr/gc/gcpriv.h
Auto-merging src/coreclr/gc/gcconfig.h
Auto-merging src/coreclr/gc/gc.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fixed spin count problem
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-04-06T22:58:51Z

@mrsharm an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

danmoseley · 2023-04-27T17:05:17Z

+ uint32_t saved_yp_spin_count_unit = yp_spin_count_unit;
+ yp_spin_count_unit = (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9);
+
+ // It's very suspicious if it becomes 0 and also, we don't want to spin too much.


if it's suspicious should it assert (yp_spin_count_unit != 0); again here

Yes, it makes sense to assert once more. Will work on adding an additional protective assert.

mrsharm · 2023-04-05T01:15:35Z

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

ghost · 2023-04-05T01:15:45Z

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

Issue Details

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

Author:	mrsharm
Assignees:	-
Labels:	`area-GC-coreclr`
Milestone:	-

mangod9 · 2023-04-05T17:24:29Z

Adding @eduardo-vp. Would be good to try out this config to ensure that setting it to something low <10, gets the performance for the app back to 6.0.5 numbers.

Maoni0 · 2023-04-05T19:20:28Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

mrsharm · 2023-04-05T19:20:42Z

@eduardo-vp, to test this out, you'll need to simply set the environment variable: DOTNET_GCSpinCountUnit to some low value in Hex. For example, I tested this with:

$env:DOTNET_GCSpinCountUnit=9

mrsharm · 2023-04-05T19:53:19Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

To test this out, I used a GCPerfSim scenario and debugged both the case where we set DOTNET_GCSpinCountUnit and when we don't by putting breakpoints in the Initialize and SetYieldProcessorScalingFactor functions.

Without The Environment Variable

In the Initialize function, we skip setting the yp_spin_count_unit to that of the config:

0:000> x coreclr!*yp_spin_count_unit*
00007ffe`3059ebcc coreclr!WKS::yp_spin_count_unit = 0

In the SetYieldProcessorScalingFactor function, we set the yp_spin_count_unit to (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9); i.e., the original logic.

With the Environment Variable Set i.e., DOTNET_GCSpinCountUnit=9

In Initialize, I confirmed:

0:000> x coreclr!WKS::*yp_spin_count*
00007ffe`3c5a5710 coreclr!WKS::yp_spin_count_unit = 9

In SetYieldProcessorScalingFactor, I confirmed, we weren't updating the yp_spin_count_unit as we were bypassing the original logic altogether as the spin_count_unit_config_p was true:

0:006> x coreclr!*gc_heap::spin_count_unit_config_p*
00007ffe`3c5a496c coreclr!WKS::gc_heap::spin_count_unit_config_p = true

Do let me know if any further tests are needed from a functional perspective.

Maoni0 · 2023-04-05T20:46:05Z

ok, the new change would work functionally but the code should be moved to next to where we are doing the normal init in initialize_gc, ie, it should be after this code -

#ifdef MULTIPLE_HEAPS
yp_spin_count_unit = 32 * number_of_heaps;
#else
yp_spin_count_unit = 32 * g_num_processors;
#endif //MULTIPLE_HEAPS

as this is now part of the initialization logic for this variable. there's no reason to separate the 2 pieces of code which means someone would need to look at 2 places instead of 1.

and re your question " if any further tests are needed from a functional perspective." - you should actually let it run and make sure that you are seeing the correct value, for example, if you had dumped the value in SetYieldProcessorScalingFactor you would've seen that it was getting the wrong value.

eduardo-vp · 2023-04-05T21:30:29Z

I ran the customer's program with the 48 concurrent instances and got these numbers

Version	Elapsed time
6.0.5 runtime (currently used by the customer)	1:57.74
Current code with DOTNET_GCSpinCountUnit unset	3:35.68
Current code with DOTNET_GCSpinCountUnit=9	2:18.84
Current code with DOTNET_GCSpinCountUnit=8	2:14.75
Current code with DOTNET_GCSpinCountUnit=7	2:04.55
Current code with DOTNET_GCSpinCountUnit=6	2:00.48
Current code with DOTNET_GCSpinCountUnit=5	1:54.24
Current code with DOTNET_GCSpinCountUnit=4	1:42.51
Current code with DOTNET_GCSpinCountUnit=3	1:40.07
Current code with DOTNET_GCSpinCountUnit=2	1:29.19
Current code with DOTNET_GCSpinCountUnit=1	1:24.38

It seems for this case the optimal is reached with DOTNET_GCSpinCountUnit=1 and it's even faster than the version they're currently using (6.0.5)

cshung

LGTM

mrsharm · 2023-04-06T22:56:39Z

/backport to release/7.0

github-actions · 2023-04-06T22:56:51Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4633715849

github-actions · 2023-04-06T22:58:50Z

@mrsharm backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixed spin count problem
Using index info to reconstruct a base tree...
M	src/coreclr/gc/gc.cpp
M	src/coreclr/gc/gcconfig.h
M	src/coreclr/gc/gcpriv.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/gc/gcpriv.h
CONFLICT (content): Merge conflict in src/coreclr/gc/gcpriv.h
Auto-merging src/coreclr/gc/gcconfig.h
Auto-merging src/coreclr/gc/gc.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fixed spin count problem
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-04-06T22:58:51Z

@mrsharm an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

danmoseley · 2023-04-27T17:05:17Z

+ uint32_t saved_yp_spin_count_unit = yp_spin_count_unit;
+ yp_spin_count_unit = (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9);
+
+ // It's very suspicious if it becomes 0 and also, we don't want to spin too much.


if it's suspicious should it assert (yp_spin_count_unit != 0); again here

Yes, it makes sense to assert once more. Will work on adding an additional protective assert.

mrsharm · 2023-04-05T01:15:35Z

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

ghost · 2023-04-05T01:15:45Z

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

Issue Details

Added the ability to specify a Spin Count Unit via a GC Configuration and make use of this value in the SetYieldProcessorScalingFactor function if the value is valid. If this configuration is not specified, we default to 0 and fall back to the original logic.

CC: @mangod9

Author:	mrsharm
Assignees:	-
Labels:	`area-GC-coreclr`
Milestone:	-

mangod9 · 2023-04-05T17:24:29Z

Adding @eduardo-vp. Would be good to try out this config to ensure that setting it to something low <10, gets the performance for the app back to 6.0.5 numbers.

Maoni0 · 2023-04-05T19:20:28Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

mrsharm · 2023-04-05T19:20:42Z

@eduardo-vp, to test this out, you'll need to simply set the environment variable: DOTNET_GCSpinCountUnit to some low value in Hex. For example, I tested this with:

$env:DOTNET_GCSpinCountUnit=9

mrsharm · 2023-04-05T19:53:19Z

how did you test this? you are reading the value from the config very early on in GCHeap::Initialize but we are doing the normal init in initialize_gc which is later. are you seeing that the value is what you specify in the config?

To test this out, I used a GCPerfSim scenario and debugged both the case where we set DOTNET_GCSpinCountUnit and when we don't by putting breakpoints in the Initialize and SetYieldProcessorScalingFactor functions.

Without The Environment Variable

In the Initialize function, we skip setting the yp_spin_count_unit to that of the config:

0:000> x coreclr!*yp_spin_count_unit*
00007ffe`3059ebcc coreclr!WKS::yp_spin_count_unit = 0

In the SetYieldProcessorScalingFactor function, we set the yp_spin_count_unit to (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9); i.e., the original logic.

With the Environment Variable Set i.e., DOTNET_GCSpinCountUnit=9

In Initialize, I confirmed:

0:000> x coreclr!WKS::*yp_spin_count*
00007ffe`3c5a5710 coreclr!WKS::yp_spin_count_unit = 9

In SetYieldProcessorScalingFactor, I confirmed, we weren't updating the yp_spin_count_unit as we were bypassing the original logic altogether as the spin_count_unit_config_p was true:

0:006> x coreclr!*gc_heap::spin_count_unit_config_p*
00007ffe`3c5a496c coreclr!WKS::gc_heap::spin_count_unit_config_p = true

Do let me know if any further tests are needed from a functional perspective.

Maoni0 · 2023-04-05T20:46:05Z

ok, the new change would work functionally but the code should be moved to next to where we are doing the normal init in initialize_gc, ie, it should be after this code -

#ifdef MULTIPLE_HEAPS
yp_spin_count_unit = 32 * number_of_heaps;
#else
yp_spin_count_unit = 32 * g_num_processors;
#endif //MULTIPLE_HEAPS

as this is now part of the initialization logic for this variable. there's no reason to separate the 2 pieces of code which means someone would need to look at 2 places instead of 1.

and re your question " if any further tests are needed from a functional perspective." - you should actually let it run and make sure that you are seeing the correct value, for example, if you had dumped the value in SetYieldProcessorScalingFactor you would've seen that it was getting the wrong value.

eduardo-vp · 2023-04-05T21:30:29Z

I ran the customer's program with the 48 concurrent instances and got these numbers

Version	Elapsed time
6.0.5 runtime (currently used by the customer)	1:57.74
Current code with DOTNET_GCSpinCountUnit unset	3:35.68
Current code with DOTNET_GCSpinCountUnit=9	2:18.84
Current code with DOTNET_GCSpinCountUnit=8	2:14.75
Current code with DOTNET_GCSpinCountUnit=7	2:04.55
Current code with DOTNET_GCSpinCountUnit=6	2:00.48
Current code with DOTNET_GCSpinCountUnit=5	1:54.24
Current code with DOTNET_GCSpinCountUnit=4	1:42.51
Current code with DOTNET_GCSpinCountUnit=3	1:40.07
Current code with DOTNET_GCSpinCountUnit=2	1:29.19
Current code with DOTNET_GCSpinCountUnit=1	1:24.38

It seems for this case the optimal is reached with DOTNET_GCSpinCountUnit=1 and it's even faster than the version they're currently using (6.0.5)

cshung

LGTM

mrsharm · 2023-04-06T22:56:39Z

/backport to release/7.0

github-actions · 2023-04-06T22:56:51Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4633715849

github-actions · 2023-04-06T22:58:50Z

@mrsharm backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixed spin count problem
Using index info to reconstruct a base tree...
M	src/coreclr/gc/gc.cpp
M	src/coreclr/gc/gcconfig.h
M	src/coreclr/gc/gcpriv.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/gc/gcpriv.h
CONFLICT (content): Merge conflict in src/coreclr/gc/gcpriv.h
Auto-merging src/coreclr/gc/gcconfig.h
Auto-merging src/coreclr/gc/gc.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fixed spin count problem
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-04-06T22:58:51Z

@mrsharm an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

danmoseley · 2023-04-27T17:05:17Z

+ uint32_t saved_yp_spin_count_unit = yp_spin_count_unit;
+ yp_spin_count_unit = (uint32_t)((float)original_spin_count_unit * scalingFactor / (float)9);
+
+ // It's very suspicious if it becomes 0 and also, we don't want to spin too much.


if it's suspicious should it assert (yp_spin_count_unit != 0); again here

Yes, it makes sense to assert once more. Will work on adding an additional protective assert.

ghost added the area-GC-coreclr label Apr 5, 2023

ghost assigned mrsharm Apr 5, 2023

mrsharm requested review from Maoni0, PeterSolMS, cshung and mangod9 April 5, 2023 01:18

build-analysisBot mentioned this pull request Apr 5, 2023
CI System.Text.RegularExpressions.Tests failure #84323
Closed

runfoappBot mentioned this pull request Apr 5, 2023
Infra improvements for Helix #68176
Closed

mangod9 approved these changes Apr 5, 2023
View reviewed changes

mrsharm marked this pull request as ready for review April 6, 2023 21:16

Maoni0 approved these changes Apr 6, 2023
View reviewed changes

cshung approved these changes Apr 6, 2023
View reviewed changes

Fixed spin count problem
70851d8

mrsharm force-pushed the spincount_fix branch from e0a6e48 to 70851d8Compare April 6, 2023 22:16

mrsharm merged commit aca3700 into dotnet:mainApr 6, 2023

mrsharm mentioned this pull request Apr 7, 2023
Added the ability to specify a Spin Count Unit via a GC Configuration to release/7.0 #84495
Merged

danmoseley reviewed Apr 27, 2023
View reviewed changes

ghost locked as resolved and limited conversation to collaborators May 28, 2023

ghost added the area-GC-coreclr label Apr 5, 2023

ghost assigned mrsharm Apr 5, 2023

mrsharm requested review from Maoni0, PeterSolMS, cshung and mangod9 April 5, 2023 01:18

build-analysisBot mentioned this pull request Apr 5, 2023
CI System.Text.RegularExpressions.Tests failure #84323
Closed

runfoappBot mentioned this pull request Apr 5, 2023
Infra improvements for Helix #68176
Closed

mangod9 approved these changes Apr 5, 2023
View reviewed changes

mrsharm marked this pull request as ready for review April 6, 2023 21:16

Maoni0 approved these changes Apr 6, 2023
View reviewed changes

cshung approved these changes Apr 6, 2023
View reviewed changes

Fixed spin count problem
70851d8

mrsharm force-pushed the spincount_fix branch from e0a6e48 to 70851d8Compare April 6, 2023 22:16

mrsharm merged commit aca3700 into dotnet:mainApr 6, 2023

mrsharm mentioned this pull request Apr 7, 2023
Added the ability to specify a Spin Count Unit via a GC Configuration to release/7.0 #84495
Merged

danmoseley reviewed Apr 27, 2023
View reviewed changes

ghost locked as resolved and limited conversation to collaborators May 28, 2023

ghost added the area-GC-coreclr label Apr 5, 2023

ghost assigned mrsharm Apr 5, 2023

mrsharm requested review from Maoni0, PeterSolMS, cshung and mangod9 April 5, 2023 01:18

build-analysisBot mentioned this pull request Apr 5, 2023
CI System.Text.RegularExpressions.Tests failure #84323
Closed

runfoappBot mentioned this pull request Apr 5, 2023
Infra improvements for Helix #68176
Closed

mangod9 approved these changes Apr 5, 2023
View reviewed changes

mrsharm marked this pull request as ready for review April 6, 2023 21:16

Maoni0 approved these changes Apr 6, 2023
View reviewed changes

cshung approved these changes Apr 6, 2023
View reviewed changes

Fixed spin count problem
70851d8

mrsharm force-pushed the spincount_fix branch from e0a6e48 to 70851d8Compare April 6, 2023 22:16

mrsharm merged commit aca3700 into dotnet:mainApr 6, 2023

mrsharm mentioned this pull request Apr 7, 2023
Added the ability to specify a Spin Count Unit via a GC Configuration to release/7.0 #84495
Merged

danmoseley reviewed Apr 27, 2023
View reviewed changes

ghost locked as resolved and limited conversation to collaborators May 28, 2023

ghost added the area-GC-coreclr label Apr 5, 2023

ghost assigned mrsharm Apr 5, 2023

mrsharm requested review from Maoni0, PeterSolMS, cshung and mangod9 April 5, 2023 01:18

build-analysisBot mentioned this pull request Apr 5, 2023
CI System.Text.RegularExpressions.Tests failure #84323
Closed

runfoappBot mentioned this pull request Apr 5, 2023
Infra improvements for Helix #68176
Closed

mangod9 approved these changes Apr 5, 2023
View reviewed changes

mrsharm marked this pull request as ready for review April 6, 2023 21:16

Maoni0 approved these changes Apr 6, 2023
View reviewed changes

cshung approved these changes Apr 6, 2023
View reviewed changes

Fixed spin count problem
70851d8

mrsharm force-pushed the spincount_fix branch from e0a6e48 to 70851d8Compare April 6, 2023 22:16

mrsharm merged commit aca3700 into dotnet:mainApr 6, 2023

mrsharm mentioned this pull request Apr 7, 2023
Added the ability to specify a Spin Count Unit via a GC Configuration to release/7.0 #84495
Merged

danmoseley reviewed Apr 27, 2023
View reviewed changes

ghost locked as resolved and limited conversation to collaborators May 28, 2023

ghost added the area-GC-coreclr label Apr 5, 2023

ghost assigned mrsharm Apr 5, 2023

mrsharm requested review from Maoni0, PeterSolMS, cshung and mangod9 April 5, 2023 01:18

build-analysisBot mentioned this pull request Apr 5, 2023
CI System.Text.RegularExpressions.Tests failure #84323
Closed

runfoappBot mentioned this pull request Apr 5, 2023
Infra improvements for Helix #68176
Closed

mangod9 approved these changes Apr 5, 2023
View reviewed changes

mrsharm marked this pull request as ready for review April 6, 2023 21:16

Maoni0 approved these changes Apr 6, 2023
View reviewed changes

cshung approved these changes Apr 6, 2023
View reviewed changes

Fixed spin count problem
70851d8

mrsharm force-pushed the spincount_fix branch from e0a6e48 to 70851d8Compare April 6, 2023 22:16

mrsharm merged commit aca3700 into dotnet:mainApr 6, 2023

mrsharm mentioned this pull request Apr 7, 2023
Added the ability to specify a Spin Count Unit via a GC Configuration to release/7.0 #84495
Merged

danmoseley reviewed Apr 27, 2023
View reviewed changes

ghost locked as resolved and limited conversation to collaborators May 28, 2023

ghost added the area-GC-coreclr label Apr 5, 2023

ghost assigned mrsharm Apr 5, 2023

mrsharm requested review from Maoni0, PeterSolMS, cshung and mangod9 April 5, 2023 01:18

build-analysisBot mentioned this pull request Apr 5, 2023
CI System.Text.RegularExpressions.Tests failure #84323
Closed

runfoappBot mentioned this pull request Apr 5, 2023
Infra improvements for Helix #68176
Closed

mangod9 approved these changes Apr 5, 2023
View reviewed changes

mrsharm marked this pull request as ready for review April 6, 2023 21:16

Maoni0 approved these changes Apr 6, 2023
View reviewed changes

cshung approved these changes Apr 6, 2023
View reviewed changes

Fixed spin count problem
70851d8

mrsharm force-pushed the spincount_fix branch from e0a6e48 to 70851d8Compare April 6, 2023 22:16

mrsharm merged commit aca3700 into dotnet:mainApr 6, 2023

mrsharm mentioned this pull request Apr 7, 2023
Added the ability to specify a Spin Count Unit via a GC Configuration to release/7.0 #84495
Merged

danmoseley reviewed Apr 27, 2023
View reviewed changes

ghost locked as resolved and limited conversation to collaborators May 28, 2023

ghost added the area-GC-coreclr label Apr 5, 2023

ghost assigned mrsharm Apr 5, 2023

mrsharm requested review from Maoni0, PeterSolMS, cshung and mangod9 April 5, 2023 01:18

build-analysisBot mentioned this pull request Apr 5, 2023
CI System.Text.RegularExpressions.Tests failure #84323
Closed

runfoappBot mentioned this pull request Apr 5, 2023
Infra improvements for Helix #68176
Closed

mangod9 approved these changes Apr 5, 2023
View reviewed changes

mrsharm marked this pull request as ready for review April 6, 2023 21:16

Maoni0 approved these changes Apr 6, 2023
View reviewed changes

cshung approved these changes Apr 6, 2023
View reviewed changes

Fixed spin count problem
70851d8

mrsharm force-pushed the spincount_fix branch from e0a6e48 to 70851d8Compare April 6, 2023 22:16

mrsharm merged commit aca3700 into dotnet:mainApr 6, 2023

mrsharm mentioned this pull request Apr 7, 2023
Added the ability to specify a Spin Count Unit via a GC Configuration to release/7.0 #84495
Merged

danmoseley reviewed Apr 27, 2023
View reviewed changes

ghost locked as resolved and limited conversation to collaborators May 28, 2023

ghost added the area-GC-coreclr label Apr 5, 2023

ghost assigned mrsharm Apr 5, 2023

mrsharm requested review from Maoni0, PeterSolMS, cshung and mangod9 April 5, 2023 01:18

build-analysisBot mentioned this pull request Apr 5, 2023
CI System.Text.RegularExpressions.Tests failure #84323
Closed

runfoappBot mentioned this pull request Apr 5, 2023
Infra improvements for Helix #68176
Closed

mangod9 approved these changes Apr 5, 2023
View reviewed changes

mrsharm marked this pull request as ready for review April 6, 2023 21:16

Maoni0 approved these changes Apr 6, 2023
View reviewed changes

cshung approved these changes Apr 6, 2023
View reviewed changes

Fixed spin count problem
70851d8

mrsharm force-pushed the spincount_fix branch from e0a6e48 to 70851d8Compare April 6, 2023 22:16

mrsharm merged commit aca3700 into dotnet:mainApr 6, 2023

mrsharm mentioned this pull request Apr 7, 2023
Added the ability to specify a Spin Count Unit via a GC Configuration to release/7.0 #84495
Merged

danmoseley reviewed Apr 27, 2023
View reviewed changes

ghost locked as resolved and limited conversation to collaborators May 28, 2023

Uh oh!

Conversation

mrsharm commented Apr 5, 2023

Uh oh!

ghost commented Apr 5, 2023

Uh oh!

mangod9 commented Apr 5, 2023

Uh oh!

Maoni0 commented Apr 5, 2023

Uh oh!

mrsharm commented Apr 5, 2023

Uh oh!

mrsharm commented Apr 5, 2023

Without The Environment Variable

With the Environment Variable Set i.e., DOTNET_GCSpinCountUnit=9

Uh oh!

Maoni0 commented Apr 5, 2023

Uh oh!

eduardo-vp commented Apr 5, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cshung left a comment

Choose a reason for hiding this comment

Uh oh!

mrsharm commented Apr 6, 2023

Uh oh!

github-actionsBot commented Apr 6, 2023

Uh oh!

github-actionsBot commented Apr 6, 2023

Uh oh!

github-actionsBot commented Apr 6, 2023

Uh oh!

danmoseleyApr 27, 2023

Choose a reason for hiding this comment

Uh oh!

mrsharmApr 28, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

mrsharm commented Apr 5, 2023

Uh oh!

ghost commented Apr 5, 2023

Uh oh!

mangod9 commented Apr 5, 2023

Uh oh!

Maoni0 commented Apr 5, 2023

Uh oh!

mrsharm commented Apr 5, 2023

Uh oh!

mrsharm commented Apr 5, 2023

Without The Environment Variable

With the Environment Variable Set i.e., DOTNET_GCSpinCountUnit=9

Uh oh!

Maoni0 commented Apr 5, 2023

Uh oh!

eduardo-vp commented Apr 5, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cshung left a comment

Choose a reason for hiding this comment

Uh oh!

mrsharm commented Apr 6, 2023

Uh oh!

github-actionsBot commented Apr 6, 2023

Uh oh!

github-actionsBot commented Apr 6, 2023

Uh oh!

github-actionsBot commented Apr 6, 2023

Uh oh!

danmoseleyApr 27, 2023

Choose a reason for hiding this comment

eduardo-vp commented Apr 5, 2023•
edited
Loading

eduardo-vp commented Apr 5, 2023•
edited
Loading

eduardo-vp commented Apr 5, 2023•
edited
Loading