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

Remove must-init requirement for GS Cookies #50702

Merged

Conversation

SingleAccretion
Copy link
Contributor

The cookie is always initialized in the prolog, where no user code is allowed.
Likewise, there should be no GC concerns with it as the prolog is not interruptible.

This change should be validated with some GCStess'ed runs.

The diffs look generally positive as expected, the regressions are because of the differences in the zero-initting strategies (e. g. here it looks like a switch from a longer loop to a shorter loop + two explicit stores).

libraries.pmi.windows.x64.checked.mch

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 67824
Total bytes of diff: 67374
Total bytes of delta: -450 (-0.66% of base)
    diff is an improvement.

Top file regressions (bytes):
          16 : 212958.dasm (0.65% of base)
           2 : 175944.dasm (0.96% of base)
           2 : 175948.dasm (0.95% of base)
           2 : 175945.dasm (0.95% of base)
           2 : 175947.dasm (0.95% of base)
           2 : 175946.dasm (0.95% of base)
           1 : 203366.dasm (0.57% of base)
           1 : 203361.dasm (0.56% of base)
           1 : 203363.dasm (0.57% of base)
           1 : 203364.dasm (0.53% of base)
           1 : 212535.dasm (0.17% of base)
           1 : 203362.dasm (0.56% of base)
           1 : 83342.dasm (0.17% of base)

Top file improvements (bytes):
         -20 : 212527.dasm (-2.77% of base)
         -19 : 212937.dasm (-2.78% of base)
         -13 : 175648.dasm (-0.98% of base)
         -10 : 203370.dasm (-1.12% of base)
         -10 : 232447.dasm (-2.39% of base)
         -10 : 83292.dasm (-1.22% of base)
         -10 : 232445.dasm (-2.92% of base)
          -9 : 232446.dasm (-2.43% of base)
          -9 : 232448.dasm (-1.94% of base)
          -9 : 203369.dasm (-1.01% of base)
          -8 : 176098.dasm (-1.66% of base)
          -8 : 176067.dasm (-1.65% of base)
          -8 : 212524.dasm (-0.28% of base)
          -8 : 145624.dasm (-1.15% of base)
          -8 : 145625.dasm (-1.15% of base)
          -7 : 212516.dasm (-0.60% of base)
          -7 : 213015.dasm (-0.56% of base)
          -7 : 178539.dasm (-3.52% of base)
          -7 : 213080.dasm (-0.37% of base)
          -6 : 83285.dasm (-2.17% of base)

cc @AndyAyersMS

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 4, 2021
@SingleAccretion SingleAccretion changed the title Removed must-init requirement for GS Cookies Remove must-init requirement for GS Cookies Apr 4, 2021
@SingleAccretion SingleAccretion force-pushed the Do-Not-Zero-Initialize-GS-Cookie branch from c77104d to e87e123 Compare April 4, 2021 20:58
@EgorBo
Copy link
Member

EgorBo commented Apr 4, 2021

/azp list

@EgorBo
Copy link
Member

EgorBo commented Apr 4, 2021

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member

We wanted to run GC stress on this.

Is it not runnable via /azp ? Anyways, started it via the UI.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Stress failures all look to be unrelated.

@AndyAyersMS
Copy link
Member

Unrelated CI issues were affecting this; going to retry the "runtime" parts.

@AndyAyersMS
Copy link
Member

Still puzzled by what CI is doing here. Am going to close/re-open to get a fresh look...

@AndyAyersMS AndyAyersMS closed this Apr 6, 2021
@AndyAyersMS AndyAyersMS reopened this Apr 6, 2021
@SingleAccretion SingleAccretion force-pushed the Do-Not-Zero-Initialize-GS-Cookie branch from e87e123 to e69e403 Compare April 6, 2021 20:08
@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Apr 6, 2021

Looked through all the logs for failures:

  1. All Jitstress ones are Assertion failed 'block->bbWeight > BB_ZERO_WEIGHT' #50743.
  2. GCStress failures appear to be Comparer_get_Default test is too slow under GCStress=3 #50594 (fixed in Simplify Comparer_get_Default runtime test #50602) and Test failure: Loader/ContextualReflection/ContextualReflection/ContextualReflection.sh #48579 (disabled in Disable TestDynamicAssembly tests #50592).

Due to the fact that this branch is based on top of a commit that does not include the aforementioned fixes, the tests are failing. I will rebase on top of 8e0b54d (so as to not pick up 245cd97 which is believed to be the trigger for #50743) and that will hopefully give us a clear run.

Sorry for not taking a closer look at this sooner :(.

@AndyAyersMS
Copy link
Member

Sorry for not taking a closer look at this sooner

No worries.

I am not planning on rerunning any stress legs here -- if the normal CI passes, I'll merge.

@AndyAyersMS
Copy link
Member

Going to bounce this once more....

@AndyAyersMS AndyAyersMS closed this Apr 7, 2021
@AndyAyersMS AndyAyersMS reopened this Apr 7, 2021
The cookie is always initialized in the prolog, where no user code is allowed.
There should be no GC concerns either as the prolog is not interruptible.
@AndyAyersMS
Copy link
Member

Finally!

Thanks again, @SingleAccretion...

@AndyAyersMS AndyAyersMS merged commit 84d9d6b into dotnet:main Apr 9, 2021
@SingleAccretion SingleAccretion deleted the Do-Not-Zero-Initialize-GS-Cookie branch April 13, 2021 00:28
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Apr 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants