Cleanup after GC info version update. - #108117

Merged
VSadov merged 16 commits into
dotnet:mainfrom
VSadov:gcVerCleanup
Nov 5, 2024
Merged

Cleanup after GC info version update.#108117
VSadov merged 16 commits into
dotnet:mainfrom
VSadov:gcVerCleanup

Conversation

@VSadov

@VSadovVSadov commented Sep 22, 2024•
edited
Loading

Copy link
Copy Markdown
Member

There should not be any functional changes. The most affected part is GC stress as there is a lot of dead code there now.
Some code moved around to tease apart reachable and unreachable scenarios.

  • remove code specific to no longer used GC info versions
  • separate major parts that deal with X86 specific scenarios from everything else
  • once we have X86 and "everything else", we can make "everything else" much simpler.
    I.E. the figuring the targets of calls and what they may return is no longer needed on non-X86 - neither at instrumentation time nor at fault time.
  • once separated, X86 part can be somewhat simpler too. (no multireg returns, etc)
  • look for more things, address TODOs

@VSadov

VSadov commented Sep 23, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

azure-pipelinesBot commented Sep 23, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

This was referenced Sep 23, 2024
Closed
Closed
Open
@VSadov

VSadov commented Sep 23, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

azure-pipelinesBot commented Sep 23, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@VSadov

VSadov commented Sep 24, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

azure-pipelinesBot commented Sep 24, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@VSadov
VSadov marked this pull request as ready for review September 24, 2024 01:19
@VSadov

VSadov commented Sep 24, 2024

Copy link
Copy Markdown
MemberAuthor

I think this is ready for review.
Cc: @jkotas@janvorli@jakobbotsch

jkotas
jkotas reviewed Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
{
_ASSERTE(!nativeCodeVersion.IsNull());

// CONSIDER: does anyone call this with fZapped == true ? are there plans?

@jkotasjkotasSep 24, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Zap was a code name for fragile NGen.

Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?

@VSadovVSadovSep 24, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I have two theories - either R2R code comes on the JIT path (and JIT does not really JIT much, maybe some fixups), then we could remove fZapped path as a part this change.
Or we do not stress R2R. There is nothing that would make that impossible, I think R2R code is writeable in the same sense as JIT code is writeable. Enabling stress over R2R would be a separate change though.

I will check what happens with R2R.

@VSadov

VSadov commented Sep 24, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

azure-pipelinesBot commented Sep 24, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@janvorli

janvorli commented Sep 24, 2024

Copy link
Copy Markdown
Member

@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.

@VSadov

VSadov commented Sep 24, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.

Some of that is because a few functions make sense only for x86, and a few are very different between x86 and "the rest", so I brought x86-specific parts closer to each other under one #if defined(TARGET_X86). I think it is easier to work with this code by concentrating on X86 or "everything else" parts separately. Moving code did contribute to extra diffs.
I'll see if I could rearrange the X86 part a bit so diffs are smaller.

@VSadov

VSadov commented Sep 24, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

azure-pipelinesBot commented Sep 24, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

VSadov
VSadov commented Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
// 5) Now, thread T can modify the stack (ex: RedirectionFrame setup) while the GC thread is scanning it.
//
// This race is now mitigated below. Where we won't initiate a stress mode GC
// for a thread in cooperative mode with an active ICF, if g_TrapReturningThreads is true.

@VSadovVSadovSep 24, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The original explanation for the g_TrapReturningThreads + active ICF scenario causing a race was somewhat questionable, so I initially removed the mitigation, but the race appears to be real.

Corrected the explanation of how the race may happen and brought the mitigation back.

@VSadov

VSadov commented Sep 25, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?

It does not look like we instrument R2R code for GC stress. The instrumentation (SetupGcCoverage) is called from JitCompileCodeLocked and that happens only on JIT-compiled code path.

I will log a follow-up issue. We should be able to GC-stress R2R code. (#108117)

@VSadovVSadov mentioned this pull request Sep 25, 2024
Open
@VSadov

VSadov commented Sep 25, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

I'll see if I could rearrange the X86 part a bit so diffs are smaller.

@janvorli - We had a lot of platform specific code and now a lot of that is gone, while X86 specific code is still in. Moving code roughly to where it was resulted in many #if defined(TARGET_X86), with no corresponding "everything else" code.

I tried to move code around to reduce diffs, but it made things mostly worse. The diffs were still big enough that VS differ could not make a lot of sense of it (I assume github diff will not be any better). So basically the rearrangements were making the file harder to read without much improvement to the diff.

The code that I moved generally stayed the same, modulo removing dead code. I was particularly careful that X86 code path did not change. In couple places I made separate X86 and non-X86 copies (DoGcStress, GCCoverageInfo::SprinkleBreakpoints) - just to make sure that X86 stays the same.

I think the easiest way to review this file is basically by reading the new code, and when in doubt finding old version and check with that.

I think the most "interesting" part is that AMD64 code path is now merged with ARM,RISC-V,etc.. and not a part of X86 path.
That is where I had to add some code - like to figure lengths of x64 instructions we need to ask disassembler. However we no longer go over all the instructions for the function/funclets, we just iterate over safe points or over interruptible ranges - the same as we did on RISC architectures. Thus we no longer need a funclet iterator and do not need to worry about disassembler being confused by codegen for switches.

The part that the stress runs are green and take roughly the same time as before give me a good deal of confidence that the code is functionally the same as before.
(and I did verify in debugger that we actually stress things :-)

@VSadov

VSadov commented Oct 15, 2024

Copy link
Copy Markdown
MemberAuthor

Rebased onto recent main.

@VSadov

VSadov commented Oct 15, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

azure-pipelinesBot commented Oct 15, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@azure-pipelines

azure-pipelinesBot commented Oct 28, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@VSadov

VSadov commented Oct 28, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

azure-pipelinesBot commented Oct 28, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@VSadov

VSadov commented Oct 30, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

azure-pipelinesBot commented Oct 30, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@VSadov

VSadov commented Nov 1, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

azure-pipelinesBot commented Nov 1, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

janvorli
janvorli approved these changes Nov 5, 2024
View reviewed changes

@janvorlijanvorli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thank you!

@VSadov

VSadov commented Nov 5, 2024

Copy link
Copy Markdown
MemberAuthor

Thanks!!

@VSadov
VSadov merged commit 552782f into dotnet:mainNov 5, 2024
@VSadov
VSadov deleted the gcVerCleanup branch November 5, 2024 19:06
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 6, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas left review comments
@janvorlijanvorlijanvorli approved these changes
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees

@VSadovVSadov

Labels

area-VM-coreclr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@VSadov@janvorli@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Cleanup after GC info version update. - #108117
Merged
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Nov 5, 2024
Merged
Cleanup after GC info version update.#108117
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Conversation
@VSadov
@VSadovVSadov
commented
Sep 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
There should not be any functional changes. The most affected part is GC stress as there is a lot of dead code there now.

Some code moved around to tease apart reachable and unreachable scenarios.
  •  remove code specific to no longer used GC info versions
  •  separate major parts that deal with X86 specific scenarios from everything else
  •  once we have X86 and "everything else", we can make "everything else" much simpler.
    
I.E. the figuring the targets of calls and what they may return is no longer needed on non-X86 - neither at instrumentation time nor at fault time.
  •  once separated, X86 part can be somewhat simpler too. (no multireg returns, etc)
  •  look for more things, address TODOs
@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).

This was referenced Sep 23, 2024

Closed


Closed


Open

@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
marked this pull request as ready for review
September 24, 2024 01:19
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
I think this is ready for review.

Cc: @jkotas@janvorli@jakobbotsch
jkotas
jkotas
reviewed
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
{
 _ASSERTE(!nativeCodeVersion.IsNull());

 // CONSIDER: does anyone call this with fZapped == true ? are there plans?
@jkotasjkotasSep 24, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Zap was a code name for fragile NGen.
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I have two theories - either R2R code comes on the JIT path (and JIT does not really JIT much, maybe some fixups), then we could remove fZapped  path as a part this change.

Or we do not stress R2R. There is nothing that would make that impossible, I think R2R code is writeable in the same sense as JIT code is writeable. Enabling stress over R2R would be a separate change though.
I will check what happens with R2R.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@janvorli
janvorli
commented
Sep 24, 2024

Copy link


Copy Markdown

Member
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
@VSadov
VSadov
commented
Sep 24, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
Some of that is because a few functions make sense only for x86, and a few are very different between x86 and "the rest", so I brought x86-specific parts closer to each other under one #if defined(TARGET_X86). I think it is easier to work with this code by concentrating on X86 or "everything else" parts separately. Moving code did contribute to extra diffs.

I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
VSadov
VSadov
commented
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
// 5) Now, thread T can modify the stack (ex: RedirectionFrame setup) while the GC thread is scanning it.
//
// This race is now mitigated below. Where we won't initiate a stress mode GC
// for a thread in cooperative mode with an active ICF, if g_TrapReturningThreads is true.
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

The original explanation for the g_TrapReturningThreads + active ICF scenario causing a race was somewhat questionable, so I initially removed the mitigation, but the race appears to be real.
Corrected the explanation of how the race may happen and brought the mitigation back.
@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
It does not look like we instrument R2R code for GC stress. The instrumentation (SetupGcCoverage) is called from JitCompileCodeLocked and that happens only on JIT-compiled code path.
I will log a follow-up issue. We should be able to GC-stress R2R code. (#108117)
@VSadovVSadov
mentioned this pull request
Sep 25, 2024

Open

@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@janvorli - We had a lot of platform specific code and now a lot of that is gone, while X86 specific code is still in. Moving code roughly to where it was resulted in many #if defined(TARGET_X86), with no corresponding "everything else" code.
I tried to move code around to reduce diffs, but it made things mostly worse. The diffs were still big enough that VS differ could not make a lot of sense of it (I assume github diff will not be any better). So basically the rearrangements were making the file harder to read without much improvement to the diff.
The code that I moved generally stayed the same, modulo removing dead code. I was particularly careful that X86 code path did not change. In couple places I made separate X86 and non-X86 copies (DoGcStress, GCCoverageInfo::SprinkleBreakpoints) - just to make sure that X86 stays the same.
I think the easiest way to review this file is basically by reading the new code, and when in doubt finding old version and check with that.
I think the most "interesting" part is that AMD64 code path is now merged with ARM,RISC-V,etc.. and not a part of X86 path.

That is where I had to add some code - like to figure lengths of x64 instructions we need to ask disassembler. However we no longer go over all the instructions for the function/funclets, we just iterate over safe points or over interruptible ranges - the same as we did on RISC architectures. Thus we no longer need a funclet iterator and do not need to worry about disassembler being confused by codegen for switches.
The part that the stress runs are green and take roughly the same time as before give me a good deal of confidence that the code is functionally the same as before.

(and I did verify in debugger that we actually stress things :-)
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
Rebased onto recent main.
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 15, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 28, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 30, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 30, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Nov 1, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Nov 1, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
janvorli
janvorli
approved these changes
Nov 5, 2024
View reviewed changes
@janvorlijanvorli
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM, thank you!
@VSadov
VSadov
commented
Nov 5, 2024

Copy link


Copy Markdown

MemberAuthor
Thanks!!
@VSadov
VSadov
merged commit 552782f
into
dotnet:mainNov 5, 2024
@VSadov
VSadov
deleted the

gcVerCleanup

branch
November 5, 2024 19:06
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 6, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@janvorlijanvorlijanvorli approved these changes
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees

@VSadovVSadov

Labels

area-VM-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@VSadov@janvorli@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Cleanup after GC info version update. - #108117
Merged
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Nov 5, 2024
Merged
Cleanup after GC info version update.#108117
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Conversation
@VSadov
@VSadovVSadov
commented
Sep 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
There should not be any functional changes. The most affected part is GC stress as there is a lot of dead code there now.

Some code moved around to tease apart reachable and unreachable scenarios.
  •  remove code specific to no longer used GC info versions
  •  separate major parts that deal with X86 specific scenarios from everything else
  •  once we have X86 and "everything else", we can make "everything else" much simpler.
    
I.E. the figuring the targets of calls and what they may return is no longer needed on non-X86 - neither at instrumentation time nor at fault time.
  •  once separated, X86 part can be somewhat simpler too. (no multireg returns, etc)
  •  look for more things, address TODOs
@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).

This was referenced Sep 23, 2024

Closed


Closed


Open

@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
marked this pull request as ready for review
September 24, 2024 01:19
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
I think this is ready for review.

Cc: @jkotas@janvorli@jakobbotsch
jkotas
jkotas
reviewed
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
{
 _ASSERTE(!nativeCodeVersion.IsNull());

 // CONSIDER: does anyone call this with fZapped == true ? are there plans?
@jkotasjkotasSep 24, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Zap was a code name for fragile NGen.
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I have two theories - either R2R code comes on the JIT path (and JIT does not really JIT much, maybe some fixups), then we could remove fZapped  path as a part this change.

Or we do not stress R2R. There is nothing that would make that impossible, I think R2R code is writeable in the same sense as JIT code is writeable. Enabling stress over R2R would be a separate change though.
I will check what happens with R2R.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@janvorli
janvorli
commented
Sep 24, 2024

Copy link


Copy Markdown

Member
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
@VSadov
VSadov
commented
Sep 24, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
Some of that is because a few functions make sense only for x86, and a few are very different between x86 and "the rest", so I brought x86-specific parts closer to each other under one #if defined(TARGET_X86). I think it is easier to work with this code by concentrating on X86 or "everything else" parts separately. Moving code did contribute to extra diffs.

I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
VSadov
VSadov
commented
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
// 5) Now, thread T can modify the stack (ex: RedirectionFrame setup) while the GC thread is scanning it.
//
// This race is now mitigated below. Where we won't initiate a stress mode GC
// for a thread in cooperative mode with an active ICF, if g_TrapReturningThreads is true.
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

The original explanation for the g_TrapReturningThreads + active ICF scenario causing a race was somewhat questionable, so I initially removed the mitigation, but the race appears to be real.
Corrected the explanation of how the race may happen and brought the mitigation back.
@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
It does not look like we instrument R2R code for GC stress. The instrumentation (SetupGcCoverage) is called from JitCompileCodeLocked and that happens only on JIT-compiled code path.
I will log a follow-up issue. We should be able to GC-stress R2R code. (#108117)
@VSadovVSadov
mentioned this pull request
Sep 25, 2024

Open

@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@janvorli - We had a lot of platform specific code and now a lot of that is gone, while X86 specific code is still in. Moving code roughly to where it was resulted in many #if defined(TARGET_X86), with no corresponding "everything else" code.
I tried to move code around to reduce diffs, but it made things mostly worse. The diffs were still big enough that VS differ could not make a lot of sense of it (I assume github diff will not be any better). So basically the rearrangements were making the file harder to read without much improvement to the diff.
The code that I moved generally stayed the same, modulo removing dead code. I was particularly careful that X86 code path did not change. In couple places I made separate X86 and non-X86 copies (DoGcStress, GCCoverageInfo::SprinkleBreakpoints) - just to make sure that X86 stays the same.
I think the easiest way to review this file is basically by reading the new code, and when in doubt finding old version and check with that.
I think the most "interesting" part is that AMD64 code path is now merged with ARM,RISC-V,etc.. and not a part of X86 path.

That is where I had to add some code - like to figure lengths of x64 instructions we need to ask disassembler. However we no longer go over all the instructions for the function/funclets, we just iterate over safe points or over interruptible ranges - the same as we did on RISC architectures. Thus we no longer need a funclet iterator and do not need to worry about disassembler being confused by codegen for switches.
The part that the stress runs are green and take roughly the same time as before give me a good deal of confidence that the code is functionally the same as before.

(and I did verify in debugger that we actually stress things :-)
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
Rebased onto recent main.
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 15, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 28, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 30, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 30, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Nov 1, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Nov 1, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
janvorli
janvorli
approved these changes
Nov 5, 2024
View reviewed changes
@janvorlijanvorli
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM, thank you!
@VSadov
VSadov
commented
Nov 5, 2024

Copy link


Copy Markdown

MemberAuthor
Thanks!!
@VSadov
VSadov
merged commit 552782f
into
dotnet:mainNov 5, 2024
@VSadov
VSadov
deleted the

gcVerCleanup

branch
November 5, 2024 19:06
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 6, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@janvorlijanvorlijanvorli approved these changes
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees

@VSadovVSadov

Labels

area-VM-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@VSadov@janvorli@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Cleanup after GC info version update. - #108117
Merged
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Nov 5, 2024
Merged
Cleanup after GC info version update.#108117
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Conversation
@VSadov
@VSadovVSadov
commented
Sep 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
There should not be any functional changes. The most affected part is GC stress as there is a lot of dead code there now.

Some code moved around to tease apart reachable and unreachable scenarios.
  •  remove code specific to no longer used GC info versions
  •  separate major parts that deal with X86 specific scenarios from everything else
  •  once we have X86 and "everything else", we can make "everything else" much simpler.
    
I.E. the figuring the targets of calls and what they may return is no longer needed on non-X86 - neither at instrumentation time nor at fault time.
  •  once separated, X86 part can be somewhat simpler too. (no multireg returns, etc)
  •  look for more things, address TODOs
@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).

This was referenced Sep 23, 2024

Closed


Closed


Open

@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
marked this pull request as ready for review
September 24, 2024 01:19
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
I think this is ready for review.

Cc: @jkotas@janvorli@jakobbotsch
jkotas
jkotas
reviewed
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
{
 _ASSERTE(!nativeCodeVersion.IsNull());

 // CONSIDER: does anyone call this with fZapped == true ? are there plans?
@jkotasjkotasSep 24, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Zap was a code name for fragile NGen.
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I have two theories - either R2R code comes on the JIT path (and JIT does not really JIT much, maybe some fixups), then we could remove fZapped  path as a part this change.

Or we do not stress R2R. There is nothing that would make that impossible, I think R2R code is writeable in the same sense as JIT code is writeable. Enabling stress over R2R would be a separate change though.
I will check what happens with R2R.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@janvorli
janvorli
commented
Sep 24, 2024

Copy link


Copy Markdown

Member
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
@VSadov
VSadov
commented
Sep 24, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
Some of that is because a few functions make sense only for x86, and a few are very different between x86 and "the rest", so I brought x86-specific parts closer to each other under one #if defined(TARGET_X86). I think it is easier to work with this code by concentrating on X86 or "everything else" parts separately. Moving code did contribute to extra diffs.

I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
VSadov
VSadov
commented
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
// 5) Now, thread T can modify the stack (ex: RedirectionFrame setup) while the GC thread is scanning it.
//
// This race is now mitigated below. Where we won't initiate a stress mode GC
// for a thread in cooperative mode with an active ICF, if g_TrapReturningThreads is true.
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

The original explanation for the g_TrapReturningThreads + active ICF scenario causing a race was somewhat questionable, so I initially removed the mitigation, but the race appears to be real.
Corrected the explanation of how the race may happen and brought the mitigation back.
@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
It does not look like we instrument R2R code for GC stress. The instrumentation (SetupGcCoverage) is called from JitCompileCodeLocked and that happens only on JIT-compiled code path.
I will log a follow-up issue. We should be able to GC-stress R2R code. (#108117)
@VSadovVSadov
mentioned this pull request
Sep 25, 2024

Open

@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@janvorli - We had a lot of platform specific code and now a lot of that is gone, while X86 specific code is still in. Moving code roughly to where it was resulted in many #if defined(TARGET_X86), with no corresponding "everything else" code.
I tried to move code around to reduce diffs, but it made things mostly worse. The diffs were still big enough that VS differ could not make a lot of sense of it (I assume github diff will not be any better). So basically the rearrangements were making the file harder to read without much improvement to the diff.
The code that I moved generally stayed the same, modulo removing dead code. I was particularly careful that X86 code path did not change. In couple places I made separate X86 and non-X86 copies (DoGcStress, GCCoverageInfo::SprinkleBreakpoints) - just to make sure that X86 stays the same.
I think the easiest way to review this file is basically by reading the new code, and when in doubt finding old version and check with that.
I think the most "interesting" part is that AMD64 code path is now merged with ARM,RISC-V,etc.. and not a part of X86 path.

That is where I had to add some code - like to figure lengths of x64 instructions we need to ask disassembler. However we no longer go over all the instructions for the function/funclets, we just iterate over safe points or over interruptible ranges - the same as we did on RISC architectures. Thus we no longer need a funclet iterator and do not need to worry about disassembler being confused by codegen for switches.
The part that the stress runs are green and take roughly the same time as before give me a good deal of confidence that the code is functionally the same as before.

(and I did verify in debugger that we actually stress things :-)
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
Rebased onto recent main.
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 15, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 28, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 30, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 30, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Nov 1, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Nov 1, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
janvorli
janvorli
approved these changes
Nov 5, 2024
View reviewed changes
@janvorlijanvorli
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM, thank you!
@VSadov
VSadov
commented
Nov 5, 2024

Copy link


Copy Markdown

MemberAuthor
Thanks!!
@VSadov
VSadov
merged commit 552782f
into
dotnet:mainNov 5, 2024
@VSadov
VSadov
deleted the

gcVerCleanup

branch
November 5, 2024 19:06
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 6, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@janvorlijanvorlijanvorli approved these changes
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees

@VSadovVSadov

Labels

area-VM-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@VSadov@janvorli@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Cleanup after GC info version update. - #108117
Merged
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Nov 5, 2024
Merged
Cleanup after GC info version update.#108117
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Conversation
@VSadov
@VSadovVSadov
commented
Sep 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
There should not be any functional changes. The most affected part is GC stress as there is a lot of dead code there now.

Some code moved around to tease apart reachable and unreachable scenarios.
  •  remove code specific to no longer used GC info versions
  •  separate major parts that deal with X86 specific scenarios from everything else
  •  once we have X86 and "everything else", we can make "everything else" much simpler.
    
I.E. the figuring the targets of calls and what they may return is no longer needed on non-X86 - neither at instrumentation time nor at fault time.
  •  once separated, X86 part can be somewhat simpler too. (no multireg returns, etc)
  •  look for more things, address TODOs
@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).

This was referenced Sep 23, 2024

Closed


Closed


Open

@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
marked this pull request as ready for review
September 24, 2024 01:19
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
I think this is ready for review.

Cc: @jkotas@janvorli@jakobbotsch
jkotas
jkotas
reviewed
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
{
 _ASSERTE(!nativeCodeVersion.IsNull());

 // CONSIDER: does anyone call this with fZapped == true ? are there plans?
@jkotasjkotasSep 24, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Zap was a code name for fragile NGen.
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I have two theories - either R2R code comes on the JIT path (and JIT does not really JIT much, maybe some fixups), then we could remove fZapped  path as a part this change.

Or we do not stress R2R. There is nothing that would make that impossible, I think R2R code is writeable in the same sense as JIT code is writeable. Enabling stress over R2R would be a separate change though.
I will check what happens with R2R.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@janvorli
janvorli
commented
Sep 24, 2024

Copy link


Copy Markdown

Member
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
@VSadov
VSadov
commented
Sep 24, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
Some of that is because a few functions make sense only for x86, and a few are very different between x86 and "the rest", so I brought x86-specific parts closer to each other under one #if defined(TARGET_X86). I think it is easier to work with this code by concentrating on X86 or "everything else" parts separately. Moving code did contribute to extra diffs.

I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
VSadov
VSadov
commented
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
// 5) Now, thread T can modify the stack (ex: RedirectionFrame setup) while the GC thread is scanning it.
//
// This race is now mitigated below. Where we won't initiate a stress mode GC
// for a thread in cooperative mode with an active ICF, if g_TrapReturningThreads is true.
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

The original explanation for the g_TrapReturningThreads + active ICF scenario causing a race was somewhat questionable, so I initially removed the mitigation, but the race appears to be real.
Corrected the explanation of how the race may happen and brought the mitigation back.
@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
It does not look like we instrument R2R code for GC stress. The instrumentation (SetupGcCoverage) is called from JitCompileCodeLocked and that happens only on JIT-compiled code path.
I will log a follow-up issue. We should be able to GC-stress R2R code. (#108117)
@VSadovVSadov
mentioned this pull request
Sep 25, 2024

Open

@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@janvorli - We had a lot of platform specific code and now a lot of that is gone, while X86 specific code is still in. Moving code roughly to where it was resulted in many #if defined(TARGET_X86), with no corresponding "everything else" code.
I tried to move code around to reduce diffs, but it made things mostly worse. The diffs were still big enough that VS differ could not make a lot of sense of it (I assume github diff will not be any better). So basically the rearrangements were making the file harder to read without much improvement to the diff.
The code that I moved generally stayed the same, modulo removing dead code. I was particularly careful that X86 code path did not change. In couple places I made separate X86 and non-X86 copies (DoGcStress, GCCoverageInfo::SprinkleBreakpoints) - just to make sure that X86 stays the same.
I think the easiest way to review this file is basically by reading the new code, and when in doubt finding old version and check with that.
I think the most "interesting" part is that AMD64 code path is now merged with ARM,RISC-V,etc.. and not a part of X86 path.

That is where I had to add some code - like to figure lengths of x64 instructions we need to ask disassembler. However we no longer go over all the instructions for the function/funclets, we just iterate over safe points or over interruptible ranges - the same as we did on RISC architectures. Thus we no longer need a funclet iterator and do not need to worry about disassembler being confused by codegen for switches.
The part that the stress runs are green and take roughly the same time as before give me a good deal of confidence that the code is functionally the same as before.

(and I did verify in debugger that we actually stress things :-)
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
Rebased onto recent main.
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 15, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 28, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 30, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 30, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Nov 1, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Nov 1, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
janvorli
janvorli
approved these changes
Nov 5, 2024
View reviewed changes
@janvorlijanvorli
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM, thank you!
@VSadov
VSadov
commented
Nov 5, 2024

Copy link


Copy Markdown

MemberAuthor
Thanks!!
@VSadov
VSadov
merged commit 552782f
into
dotnet:mainNov 5, 2024
@VSadov
VSadov
deleted the

gcVerCleanup

branch
November 5, 2024 19:06
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 6, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@janvorlijanvorlijanvorli approved these changes
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees

@VSadovVSadov

Labels

area-VM-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@VSadov@janvorli@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Cleanup after GC info version update. - #108117
Merged
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Nov 5, 2024
Merged
Cleanup after GC info version update.#108117
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Conversation
@VSadov
@VSadovVSadov
commented
Sep 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
There should not be any functional changes. The most affected part is GC stress as there is a lot of dead code there now.

Some code moved around to tease apart reachable and unreachable scenarios.
  •  remove code specific to no longer used GC info versions
  •  separate major parts that deal with X86 specific scenarios from everything else
  •  once we have X86 and "everything else", we can make "everything else" much simpler.
    
I.E. the figuring the targets of calls and what they may return is no longer needed on non-X86 - neither at instrumentation time nor at fault time.
  •  once separated, X86 part can be somewhat simpler too. (no multireg returns, etc)
  •  look for more things, address TODOs
@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).

This was referenced Sep 23, 2024

Closed


Closed


Open

@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
marked this pull request as ready for review
September 24, 2024 01:19
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
I think this is ready for review.

Cc: @jkotas@janvorli@jakobbotsch
jkotas
jkotas
reviewed
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
{
 _ASSERTE(!nativeCodeVersion.IsNull());

 // CONSIDER: does anyone call this with fZapped == true ? are there plans?
@jkotasjkotasSep 24, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Zap was a code name for fragile NGen.
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I have two theories - either R2R code comes on the JIT path (and JIT does not really JIT much, maybe some fixups), then we could remove fZapped  path as a part this change.

Or we do not stress R2R. There is nothing that would make that impossible, I think R2R code is writeable in the same sense as JIT code is writeable. Enabling stress over R2R would be a separate change though.
I will check what happens with R2R.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@janvorli
janvorli
commented
Sep 24, 2024

Copy link


Copy Markdown

Member
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
@VSadov
VSadov
commented
Sep 24, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
Some of that is because a few functions make sense only for x86, and a few are very different between x86 and "the rest", so I brought x86-specific parts closer to each other under one #if defined(TARGET_X86). I think it is easier to work with this code by concentrating on X86 or "everything else" parts separately. Moving code did contribute to extra diffs.

I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
VSadov
VSadov
commented
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
// 5) Now, thread T can modify the stack (ex: RedirectionFrame setup) while the GC thread is scanning it.
//
// This race is now mitigated below. Where we won't initiate a stress mode GC
// for a thread in cooperative mode with an active ICF, if g_TrapReturningThreads is true.
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

The original explanation for the g_TrapReturningThreads + active ICF scenario causing a race was somewhat questionable, so I initially removed the mitigation, but the race appears to be real.
Corrected the explanation of how the race may happen and brought the mitigation back.
@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
It does not look like we instrument R2R code for GC stress. The instrumentation (SetupGcCoverage) is called from JitCompileCodeLocked and that happens only on JIT-compiled code path.
I will log a follow-up issue. We should be able to GC-stress R2R code. (#108117)
@VSadovVSadov
mentioned this pull request
Sep 25, 2024

Open

@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@janvorli - We had a lot of platform specific code and now a lot of that is gone, while X86 specific code is still in. Moving code roughly to where it was resulted in many #if defined(TARGET_X86), with no corresponding "everything else" code.
I tried to move code around to reduce diffs, but it made things mostly worse. The diffs were still big enough that VS differ could not make a lot of sense of it (I assume github diff will not be any better). So basically the rearrangements were making the file harder to read without much improvement to the diff.
The code that I moved generally stayed the same, modulo removing dead code. I was particularly careful that X86 code path did not change. In couple places I made separate X86 and non-X86 copies (DoGcStress, GCCoverageInfo::SprinkleBreakpoints) - just to make sure that X86 stays the same.
I think the easiest way to review this file is basically by reading the new code, and when in doubt finding old version and check with that.
I think the most "interesting" part is that AMD64 code path is now merged with ARM,RISC-V,etc.. and not a part of X86 path.

That is where I had to add some code - like to figure lengths of x64 instructions we need to ask disassembler. However we no longer go over all the instructions for the function/funclets, we just iterate over safe points or over interruptible ranges - the same as we did on RISC architectures. Thus we no longer need a funclet iterator and do not need to worry about disassembler being confused by codegen for switches.
The part that the stress runs are green and take roughly the same time as before give me a good deal of confidence that the code is functionally the same as before.

(and I did verify in debugger that we actually stress things :-)
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
Rebased onto recent main.
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 15, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 28, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 30, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 30, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Nov 1, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Nov 1, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
janvorli
janvorli
approved these changes
Nov 5, 2024
View reviewed changes
@janvorlijanvorli
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM, thank you!
@VSadov
VSadov
commented
Nov 5, 2024

Copy link


Copy Markdown

MemberAuthor
Thanks!!
@VSadov
VSadov
merged commit 552782f
into
dotnet:mainNov 5, 2024
@VSadov
VSadov
deleted the

gcVerCleanup

branch
November 5, 2024 19:06
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 6, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@janvorlijanvorlijanvorli approved these changes
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees

@VSadovVSadov

Labels

area-VM-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@VSadov@janvorli@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Cleanup after GC info version update. - #108117
Merged
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Nov 5, 2024
Merged
Cleanup after GC info version update.#108117
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Conversation
@VSadov
@VSadovVSadov
commented
Sep 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
There should not be any functional changes. The most affected part is GC stress as there is a lot of dead code there now.

Some code moved around to tease apart reachable and unreachable scenarios.
  •  remove code specific to no longer used GC info versions
  •  separate major parts that deal with X86 specific scenarios from everything else
  •  once we have X86 and "everything else", we can make "everything else" much simpler.
    
I.E. the figuring the targets of calls and what they may return is no longer needed on non-X86 - neither at instrumentation time nor at fault time.
  •  once separated, X86 part can be somewhat simpler too. (no multireg returns, etc)
  •  look for more things, address TODOs
@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).

This was referenced Sep 23, 2024

Closed


Closed


Open

@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
marked this pull request as ready for review
September 24, 2024 01:19
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
I think this is ready for review.

Cc: @jkotas@janvorli@jakobbotsch
jkotas
jkotas
reviewed
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
{
 _ASSERTE(!nativeCodeVersion.IsNull());

 // CONSIDER: does anyone call this with fZapped == true ? are there plans?
@jkotasjkotasSep 24, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Zap was a code name for fragile NGen.
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I have two theories - either R2R code comes on the JIT path (and JIT does not really JIT much, maybe some fixups), then we could remove fZapped  path as a part this change.

Or we do not stress R2R. There is nothing that would make that impossible, I think R2R code is writeable in the same sense as JIT code is writeable. Enabling stress over R2R would be a separate change though.
I will check what happens with R2R.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@janvorli
janvorli
commented
Sep 24, 2024

Copy link


Copy Markdown

Member
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
@VSadov
VSadov
commented
Sep 24, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
Some of that is because a few functions make sense only for x86, and a few are very different between x86 and "the rest", so I brought x86-specific parts closer to each other under one #if defined(TARGET_X86). I think it is easier to work with this code by concentrating on X86 or "everything else" parts separately. Moving code did contribute to extra diffs.

I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
VSadov
VSadov
commented
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
// 5) Now, thread T can modify the stack (ex: RedirectionFrame setup) while the GC thread is scanning it.
//
// This race is now mitigated below. Where we won't initiate a stress mode GC
// for a thread in cooperative mode with an active ICF, if g_TrapReturningThreads is true.
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

The original explanation for the g_TrapReturningThreads + active ICF scenario causing a race was somewhat questionable, so I initially removed the mitigation, but the race appears to be real.
Corrected the explanation of how the race may happen and brought the mitigation back.
@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
It does not look like we instrument R2R code for GC stress. The instrumentation (SetupGcCoverage) is called from JitCompileCodeLocked and that happens only on JIT-compiled code path.
I will log a follow-up issue. We should be able to GC-stress R2R code. (#108117)
@VSadovVSadov
mentioned this pull request
Sep 25, 2024

Open

@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@janvorli - We had a lot of platform specific code and now a lot of that is gone, while X86 specific code is still in. Moving code roughly to where it was resulted in many #if defined(TARGET_X86), with no corresponding "everything else" code.
I tried to move code around to reduce diffs, but it made things mostly worse. The diffs were still big enough that VS differ could not make a lot of sense of it (I assume github diff will not be any better). So basically the rearrangements were making the file harder to read without much improvement to the diff.
The code that I moved generally stayed the same, modulo removing dead code. I was particularly careful that X86 code path did not change. In couple places I made separate X86 and non-X86 copies (DoGcStress, GCCoverageInfo::SprinkleBreakpoints) - just to make sure that X86 stays the same.
I think the easiest way to review this file is basically by reading the new code, and when in doubt finding old version and check with that.
I think the most "interesting" part is that AMD64 code path is now merged with ARM,RISC-V,etc.. and not a part of X86 path.

That is where I had to add some code - like to figure lengths of x64 instructions we need to ask disassembler. However we no longer go over all the instructions for the function/funclets, we just iterate over safe points or over interruptible ranges - the same as we did on RISC architectures. Thus we no longer need a funclet iterator and do not need to worry about disassembler being confused by codegen for switches.
The part that the stress runs are green and take roughly the same time as before give me a good deal of confidence that the code is functionally the same as before.

(and I did verify in debugger that we actually stress things :-)
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
Rebased onto recent main.
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 15, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 28, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 30, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 30, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Nov 1, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Nov 1, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
janvorli
janvorli
approved these changes
Nov 5, 2024
View reviewed changes
@janvorlijanvorli
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM, thank you!
@VSadov
VSadov
commented
Nov 5, 2024

Copy link


Copy Markdown

MemberAuthor
Thanks!!
@VSadov
VSadov
merged commit 552782f
into
dotnet:mainNov 5, 2024
@VSadov
VSadov
deleted the

gcVerCleanup

branch
November 5, 2024 19:06
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 6, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@janvorlijanvorlijanvorli approved these changes
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees

@VSadovVSadov

Labels

area-VM-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@VSadov@janvorli@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Cleanup after GC info version update. - #108117
Merged
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Nov 5, 2024
Merged
Cleanup after GC info version update.#108117
VSadov merged 16 commits into
dotnet:mainfrom 
VSadov:gcVerCleanup
Conversation
@VSadov
@VSadovVSadov
commented
Sep 22, 2024•

edited

Loading

Copy link


Copy Markdown

Member
There should not be any functional changes. The most affected part is GC stress as there is a lot of dead code there now.

Some code moved around to tease apart reachable and unreachable scenarios.
  •  remove code specific to no longer used GC info versions
  •  separate major parts that deal with X86 specific scenarios from everything else
  •  once we have X86 and "everything else", we can make "everything else" much simpler.
    
I.E. the figuring the targets of calls and what they may return is no longer needed on non-X86 - neither at instrumentation time nor at fault time.
  •  once separated, X86 part can be somewhat simpler too. (no multireg returns, etc)
  •  look for more things, address TODOs
@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).

This was referenced Sep 23, 2024

Closed


Closed


Open

@VSadov
VSadov
commented
Sep 23, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 23, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
marked this pull request as ready for review
September 24, 2024 01:19
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
I think this is ready for review.

Cc: @jkotas@janvorli@jakobbotsch
jkotas
jkotas
reviewed
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
{
 _ASSERTE(!nativeCodeVersion.IsNull());

 // CONSIDER: does anyone call this with fZapped == true ? are there plans?
@jkotasjkotasSep 24, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Zap was a code name for fragile NGen.
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I have two theories - either R2R code comes on the JIT path (and JIT does not really JIT much, maybe some fixups), then we could remove fZapped  path as a part this change.

Or we do not stress R2R. There is nothing that would make that impossible, I think R2R code is writeable in the same sense as JIT code is writeable. Enabling stress over R2R would be a separate change though.
I will check what happens with R2R.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@janvorli
janvorli
commented
Sep 24, 2024

Copy link


Copy Markdown

Member
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
@VSadov
VSadov
commented
Sep 24, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@VSadov I wonder if it would be possible to reorder stuff in the gccover.cpp so that the diff github shows would make sense. Looking at it now, it is very difficult to see what was changed and what was just moved.
Some of that is because a few functions make sense only for x86, and a few are very different between x86 and "the rest", so I brought x86-specific parts closer to each other under one #if defined(TARGET_X86). I think it is easier to work with this code by concentrating on X86 or "everything else" parts separately. Moving code did contribute to extra diffs.

I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@VSadov
VSadov
commented
Sep 24, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Sep 24, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
VSadov
VSadov
commented
Sep 24, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/gccover.cpp
// 5) Now, thread T can modify the stack (ex: RedirectionFrame setup) while the GC thread is scanning it.
//
// This race is now mitigated below. Where we won't initiate a stress mode GC
// for a thread in cooperative mode with an active ICF, if g_TrapReturningThreads is true.
@VSadovVSadovSep 24, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

The original explanation for the g_TrapReturningThreads + active ICF scenario causing a race was somewhat questionable, so I initially removed the mitigation, but the race appears to be real.
Corrected the explanation of how the race may happen and brought the mitigation back.
@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Does the GC stress instrumentation work for R2R code? If it does, this flag can be deleted. If it does not, what needs to happen to enable the GC stress instrumentation for R2R code?
It does not look like we instrument R2R code for GC stress. The instrumentation (SetupGcCoverage) is called from JitCompileCodeLocked and that happens only on JIT-compiled code path.
I will log a follow-up issue. We should be able to GC-stress R2R code. (#108117)
@VSadovVSadov
mentioned this pull request
Sep 25, 2024

Open

@VSadov
VSadov
commented
Sep 25, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll see if I could rearrange the X86 part a bit so diffs are smaller.
@janvorli - We had a lot of platform specific code and now a lot of that is gone, while X86 specific code is still in. Moving code roughly to where it was resulted in many #if defined(TARGET_X86), with no corresponding "everything else" code.
I tried to move code around to reduce diffs, but it made things mostly worse. The diffs were still big enough that VS differ could not make a lot of sense of it (I assume github diff will not be any better). So basically the rearrangements were making the file harder to read without much improvement to the diff.
The code that I moved generally stayed the same, modulo removing dead code. I was particularly careful that X86 code path did not change. In couple places I made separate X86 and non-X86 copies (DoGcStress, GCCoverageInfo::SprinkleBreakpoints) - just to make sure that X86 stays the same.
I think the easiest way to review this file is basically by reading the new code, and when in doubt finding old version and check with that.
I think the most "interesting" part is that AMD64 code path is now merged with ARM,RISC-V,etc.. and not a part of X86 path.

That is where I had to add some code - like to figure lengths of x64 instructions we need to ask disassembler. However we no longer go over all the instructions for the function/funclets, we just iterate over safe points or over interruptible ranges - the same as we did on RISC architectures. Thus we no longer need a funclet iterator and do not need to worry about disassembler being confused by codegen for switches.
The part that the stress runs are green and take roughly the same time as before give me a good deal of confidence that the code is functionally the same as before.

(and I did verify in debugger that we actually stress things :-)
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
Rebased onto recent main.
@VSadov
VSadov
commented
Oct 15, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 15, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 28, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 28, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Oct 30, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Oct 30, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
@VSadov
VSadov
commented
Nov 1, 2024

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines
azure-pipelinesBot
commented
Nov 1, 2024

Copy link


Copy Markdown


Azure Pipelines successfully started running 2 pipeline(s).
janvorli
janvorli
approved these changes
Nov 5, 2024
View reviewed changes
@janvorlijanvorli
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM, thank you!
@VSadov
VSadov
commented
Nov 5, 2024

Copy link


Copy Markdown

MemberAuthor
Thanks!!
@VSadov
VSadov
merged commit 552782f
into
dotnet:mainNov 5, 2024
@VSadov
VSadov
deleted the

gcVerCleanup

branch
November 5, 2024 19:06
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Dec 6, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@janvorlijanvorlijanvorli approved these changes
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees

@VSadovVSadov

Labels

area-VM-coreclr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@VSadov@janvorli@jkotas