Search native code in all R2R ni.dll in version bubble - #57277

Closed
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom
y-yamshchikov:partial-fix-44948
Closed

Search native code in all R2R ni.dll in version bubble#57277
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom
y-yamshchikov:partial-fix-44948

Conversation

@y-yamshchikov

@y-yamshchikovy-yamshchikov commented Aug 12, 2021

Copy link
Copy Markdown
Contributor

This PR fixes part of #44948 and fixes#46160.

This code simply traverses through assemblies in the application
domain. For each assembly (module) it realizes is it Ready To Run and is
it in the same bubble with (does it deliberately bubbling the) module
from which generic function originates. If so, it makes request for code
is Ready To Run and hopes there is some in the module. If the request
succeeds it proceeds with found pointer to the bare native code.

Generic instantiations like System.Collections.Concurrent.ConcurrentDictionary`2[System.Int32,System.__Canon] contained within same version bubble are either a bug in crossgen2 or should be taken care of by having PGO data.

Now such methods use their Ready To Run code.

We have got significant performance gain on startup: 7% average on our representative set on Tizen.

This PR worked out notices in PR below:
#47269
about linear search through the set of RangeSections. In this new PR we propose storing of RangeSections in sorted array (with number of optimizations inspired by prior linked list based solution).

We have extensively tested this PR on armel/Tizen platform so in this case we are confident in reliability and profitability of the solution.

Dear colleagues @jkotas@alpencolt@gbalykov@t-mustafin, please take a look.

@ghostghost added area-ReadyToRun community-contribution Indicates that the PR has been added by a community member labels Aug 12, 2021
@y-yamshchikovy-yamshchikov changed the title Partial fix 44948Search native code in all R2R ni.dll in version bubbleAug 12, 2021
@y-yamshchikov
y-yamshchikovforce-pushed the partial-fix-44948 branch 7 times, most recently from c2d2f5d to 91e753aCompareAugust 13, 2021 17:50
gbalykov
gbalykov suggested changes Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.h Outdated
Comment threadsrc/coreclr/vm/codeman.h Outdated
Comment threadsrc/coreclr/vm/prestub.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.h Outdated
Comment threadsrc/coreclr/vm/codeman.h Outdated
Comment threadsrc/coreclr/vm/codeman.h Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
@jaredparjaredpar mentioned this pull request Aug 17, 2021
Closed
gbalykov
gbalykov suggested changes Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
@y-yamshchikov
y-yamshchikovforce-pushed the partial-fix-44948 branch 3 times, most recently from da2f1cd to 8dc0aa9CompareSeptember 3, 2021 12:50
@alpencolt

alpencolt commented Sep 3, 2021

Copy link
Copy Markdown

@jkotas had you time to check PR?

jkotas
jkotas reviewed Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h Outdated

@jkotasjkotasSep 3, 2021

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.

This needs comment on what this returns. It does not return a simple index...

@jkotasjkotasSep 3, 2021

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.

Also, can it be in the private section (before public:)?

jkotas
jkotas reviewed Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h Outdated

@jkotasjkotasSep 3, 2021

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.

In debug/checked builds, we should make the initial size small and make the array grow on element at a time to ensure that the array resize algorithm and the potential race conditions are exercised.

jkotas
jkotas reviewed Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h Outdated

@jkotasjkotasSep 3, 2021

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.

Why does this need to be volatile?

jkotas
jkotas reviewed Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h Outdated

@jkotasjkotasSep 3, 2021

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.

I do not think that this needs to store HighAddress. It can just store the LowAddress and the method that is binary searching inside the array can just return the candidate RS index. We can then validate that the candidate RS index actually fits by comparing the address with RangeSection::HighAddress.

@jkotasjkotasSep 3, 2021

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.

Side-effect of this change is going to be that the size of this struct will be power of two that will make the binary search faster.

jkotas
jkotas reviewed Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp Outdated

@jkotasjkotasSep 3, 2021

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.

BEGIN_PRESERVE_LAST_ERROR should be before the newly added code.

jkotas
jkotas reviewed Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp Outdated

@jkotasjkotasSep 3, 2021•
edited
Loading

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.

It does not look right to create a dummy ExternalMethodFrame just to get the return address.

The fixups from R2R modules should come via ExternalMethodFixupWorker. Can we just use the module that is computed there?

@jkotas

jkotas commented Sep 3, 2021

Copy link
Copy Markdown
Member

RangeSections now stored in sorted array

BTW: I do like this change - it should be general performance improvement for any scenario with large number of modules.

@y-yamshchikov
y-yamshchikovforce-pushed the partial-fix-44948 branch 5 times, most recently from 3d7cad1 to fe70768CompareSeptember 16, 2021 11:24
@runfoapprunfoappBot mentioned this pull request Oct 13, 2021
Closed
jkotas
jkotas reviewed Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp Outdated

@jkotasjkotasOct 18, 2021

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.

It is important for this path to be lock-free. Adding a lock here is very likely to regress GC stackwalking performance on machines with a lot of cores.

@y-yamshchikovy-yamshchikovOct 19, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

If it is so important, we will investigate another way to isolate readers from writers. We think we could find one with two interchanging arrays.

jkotas
jkotas reviewed Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp Outdated

@jkotasjkotasOct 18, 2021

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.

I am not sure what this is trying to achieve, but it does not look right.

@y-yamshchikovy-yamshchikovOct 19, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We are obliged to ask if the lock has been aquired after NoHostCalls constructor of ReaderLockHolder. NoHostCalls is necessary because of contract requirement from very above on call stack. We have encountered contract violation if we use default constructor (and the source of violation is not GetRangeSection, it is much higher).

@mangod9

mangod9 commented Nov 15, 2021

Copy link
Copy Markdown
Member

hi @y-yamshchikov, assume you are still working on this PR feedback?

@mangod9

mangod9 commented Jan 10, 2022

Copy link
Copy Markdown
Member

hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx

@y-yamshchikov

y-yamshchikov commented Jan 13, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx

Dear @mangod9, we are working on new approach granting lockfree reading of RangeSections and still benefiting from binary search capabilities of sorted array, fighting all the contradictions dictating by the multiprocessor environment. We end up with new version of synchronization mechanics and for now working on comprehensive testing environment which grants extensive coverage of all possible synchronization issues. To be presented soon, both the code and the testing model.

@mangod9

mangod9 commented Jan 13, 2022

Copy link
Copy Markdown
Member

thanks for the update, should we close this PR for now and open a new one once ready? Thx!

@y-yamshchikov

y-yamshchikov commented Jan 15, 2022

Copy link
Copy Markdown
ContributorAuthor

thanks for the update, should we close this PR for now and open a new one once ready? Thx!

There is lot of useful feedback conversation still not outdated, I think the change is made in a successive way to prolong current PR.

@mangod9

mangod9 commented Feb 7, 2022

Copy link
Copy Markdown
Member

I would suggest we close the PR and reopen when more changes are added. This helps to keep the "Open PRs" list manageable.

@mangod9

mangod9 commented Mar 14, 2022

Copy link
Copy Markdown
Member

Closing for now so it doesnt get flagged in stale PRs. Please reopen when ready to merge again. Thx.

@mangod9mangod9 closed this Mar 14, 2022
@ghostghost locked as resolved and limited conversation to collaborators Apr 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas left review comments
+1 more reviewer
@gbalykovgbalykovgbalykov requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Version Bubble implementation flaw: some methods are forced to rejit

5 participants

@y-yamshchikov@alpencolt@jkotas@mangod9@gbalykov
, '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
Search native code in all R2R ni.dll in version bubble - #57277
Closed
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Closed
Search native code in all R2R ni.dll in version bubble#57277
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Conversation
@y-yamshchikov
@y-yamshchikovy-yamshchikov
commented
Aug 12, 2021

Copy link


Copy Markdown

Contributor
This PR fixes part of #44948 and fixes#46160.
This code simply traverses through assemblies in the application

domain. For each assembly (module) it realizes is it Ready To Run and is

it in the same bubble with (does it deliberately bubbling the) module

from which generic function originates. If so, it makes request for code

is Ready To Run and hopes there is some in the module. If the request

succeeds it proceeds with found pointer to the bare native code.
Generic instantiations like System.Collections.Concurrent.ConcurrentDictionary`2[System.Int32,System.__Canon] contained within same version bubble are either a bug in crossgen2 or should be taken care of by having PGO data.

Now such methods use their Ready To Run code.
We have got significant performance gain on startup: 7% average on our representative set on Tizen.
This PR worked out notices in PR below:
#47269

about linear search through the set of RangeSections. In this new PR we propose storing of RangeSections in sorted array (with number of optimizations inspired by prior linked list based solution).
We have extensively tested this PR on armel/Tizen platform so in this case we are confident in reliability and profitability of the solution.
Dear colleagues @jkotas@alpencolt@gbalykov@t-mustafin, please take a look.
@ghostghost
added

area-ReadyToRun

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 12, 2021
@y-yamshchikovy-yamshchikov
changed the title
Partial fix 44948Search native code in all R2R ni.dll in version bubbleAug 12, 2021
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
7 times, most recently
from
c2d2f5d to
91e753aCompareAugust 13, 2021 17:50
gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jaredparjaredpar
mentioned this pull request
Aug 17, 2021

Closed

gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
3 times, most recently
from
da2f1cd to
8dc0aa9CompareSeptember 3, 2021 12:50
@alpencolt
alpencolt
commented
Sep 3, 2021

Copy link


Copy Markdown

@jkotas had you time to check PR?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

This needs comment on what this returns. It does not return a simple index...
@jkotasjkotasSep 3, 2021

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.

Also, can it be in the private section (before public:)?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

In debug/checked builds, we should make the initial size small and make the array grow on element at a time to ensure that the array resize algorithm and the potential race conditions are exercised.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

Why does this need to be volatile?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

I do not think that this needs to store HighAddress. It can just store the LowAddress and the method that is binary searching inside the array can just return the candidate RS index. We can then validate that the candidate RS index actually fits by comparing the address with RangeSection::HighAddress.
@jkotasjkotasSep 3, 2021

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.

Side-effect of this change is going to be that the size of this struct will be power of two that will make the binary search faster.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021

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.

BEGIN_PRESERVE_LAST_ERROR should be before the newly added code.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021•

edited

Loading

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.

It does not look right to create a dummy ExternalMethodFrame just to get the return address.
The fixups from R2R modules should come via ExternalMethodFixupWorker. Can we just use the module that is computed there?
@jkotas
jkotas
commented
Sep 3, 2021

Copy link


Copy Markdown

Member
RangeSections now stored in sorted array
BTW: I do like this change - it should be general performance improvement for any scenario with large number of modules.
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
5 times, most recently
from
3d7cad1 to
fe70768CompareSeptember 16, 2021 11:24
@runfoapprunfoappBot
mentioned this pull request
Oct 13, 2021

Closed

jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

It is important for this path to be lock-free. Adding a lock here is very likely to regress GC stackwalking performance on machines with a lot of cores.
@y-yamshchikovy-yamshchikovOct 19, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

If it is so important, we will investigate another way to isolate readers from writers. We think we could find one with two interchanging arrays.
jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

I am not sure what this is trying to achieve, but it does not look right.
@y-yamshchikovy-yamshchikovOct 19, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We are obliged to ask if the lock has been aquired after NoHostCalls constructor of ReaderLockHolder. NoHostCalls is necessary because of contract requirement from very above on call stack. We have encountered contract violation if we use default constructor (and the source of violation is not GetRangeSection, it is much higher).
@mangod9
mangod9
commented
Nov 15, 2021

Copy link


Copy Markdown

Member
hi @y-yamshchikov, assume you are still working on this PR feedback?
@mangod9
mangod9
commented
Jan 10, 2022

Copy link


Copy Markdown

Member
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
@y-yamshchikov
y-yamshchikov
commented
Jan 13, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
Dear @mangod9, we are working on new approach granting lockfree reading of RangeSections and still benefiting from binary search capabilities of sorted array, fighting all the contradictions dictating by the multiprocessor environment. We end up with new version of synchronization mechanics and for now working on comprehensive testing environment which grants extensive coverage of all possible synchronization issues. To be presented soon, both the code and the testing model.
@mangod9
mangod9
commented
Jan 13, 2022

Copy link


Copy Markdown

Member
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
@y-yamshchikov
y-yamshchikov
commented
Jan 15, 2022

Copy link


Copy Markdown

ContributorAuthor
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
There is lot of useful feedback conversation still not outdated, I think the change is made in a successive way to prolong current PR.
@mangod9
mangod9
commented
Feb 7, 2022

Copy link


Copy Markdown

Member
I would suggest we close the PR and reopen when more changes are added. This helps to keep the "Open PRs" list manageable.
@mangod9
mangod9
commented
Mar 14, 2022

Copy link


Copy Markdown

Member
Closing for now so it doesnt get flagged in stale PRs. Please reopen when ready to merge again. Thx.
@mangod9mangod9
closed this
Mar 14, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Apr 13, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments

+1 more reviewer

@gbalykovgbalykovgbalykov requested changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Version Bubble implementation flaw: some methods are forced to rejit


5 participants

@y-yamshchikov@alpencolt@jkotas@mangod9@gbalykov

, '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
Search native code in all R2R ni.dll in version bubble - #57277
Closed
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Closed
Search native code in all R2R ni.dll in version bubble#57277
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Conversation
@y-yamshchikov
@y-yamshchikovy-yamshchikov
commented
Aug 12, 2021

Copy link


Copy Markdown

Contributor
This PR fixes part of #44948 and fixes#46160.
This code simply traverses through assemblies in the application

domain. For each assembly (module) it realizes is it Ready To Run and is

it in the same bubble with (does it deliberately bubbling the) module

from which generic function originates. If so, it makes request for code

is Ready To Run and hopes there is some in the module. If the request

succeeds it proceeds with found pointer to the bare native code.
Generic instantiations like System.Collections.Concurrent.ConcurrentDictionary`2[System.Int32,System.__Canon] contained within same version bubble are either a bug in crossgen2 or should be taken care of by having PGO data.

Now such methods use their Ready To Run code.
We have got significant performance gain on startup: 7% average on our representative set on Tizen.
This PR worked out notices in PR below:
#47269

about linear search through the set of RangeSections. In this new PR we propose storing of RangeSections in sorted array (with number of optimizations inspired by prior linked list based solution).
We have extensively tested this PR on armel/Tizen platform so in this case we are confident in reliability and profitability of the solution.
Dear colleagues @jkotas@alpencolt@gbalykov@t-mustafin, please take a look.
@ghostghost
added

area-ReadyToRun

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 12, 2021
@y-yamshchikovy-yamshchikov
changed the title
Partial fix 44948Search native code in all R2R ni.dll in version bubbleAug 12, 2021
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
7 times, most recently
from
c2d2f5d to
91e753aCompareAugust 13, 2021 17:50
gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jaredparjaredpar
mentioned this pull request
Aug 17, 2021

Closed

gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
3 times, most recently
from
da2f1cd to
8dc0aa9CompareSeptember 3, 2021 12:50
@alpencolt
alpencolt
commented
Sep 3, 2021

Copy link


Copy Markdown

@jkotas had you time to check PR?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

This needs comment on what this returns. It does not return a simple index...
@jkotasjkotasSep 3, 2021

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.

Also, can it be in the private section (before public:)?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

In debug/checked builds, we should make the initial size small and make the array grow on element at a time to ensure that the array resize algorithm and the potential race conditions are exercised.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

Why does this need to be volatile?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

I do not think that this needs to store HighAddress. It can just store the LowAddress and the method that is binary searching inside the array can just return the candidate RS index. We can then validate that the candidate RS index actually fits by comparing the address with RangeSection::HighAddress.
@jkotasjkotasSep 3, 2021

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.

Side-effect of this change is going to be that the size of this struct will be power of two that will make the binary search faster.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021

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.

BEGIN_PRESERVE_LAST_ERROR should be before the newly added code.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021•

edited

Loading

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.

It does not look right to create a dummy ExternalMethodFrame just to get the return address.
The fixups from R2R modules should come via ExternalMethodFixupWorker. Can we just use the module that is computed there?
@jkotas
jkotas
commented
Sep 3, 2021

Copy link


Copy Markdown

Member
RangeSections now stored in sorted array
BTW: I do like this change - it should be general performance improvement for any scenario with large number of modules.
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
5 times, most recently
from
3d7cad1 to
fe70768CompareSeptember 16, 2021 11:24
@runfoapprunfoappBot
mentioned this pull request
Oct 13, 2021

Closed

jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

It is important for this path to be lock-free. Adding a lock here is very likely to regress GC stackwalking performance on machines with a lot of cores.
@y-yamshchikovy-yamshchikovOct 19, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

If it is so important, we will investigate another way to isolate readers from writers. We think we could find one with two interchanging arrays.
jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

I am not sure what this is trying to achieve, but it does not look right.
@y-yamshchikovy-yamshchikovOct 19, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We are obliged to ask if the lock has been aquired after NoHostCalls constructor of ReaderLockHolder. NoHostCalls is necessary because of contract requirement from very above on call stack. We have encountered contract violation if we use default constructor (and the source of violation is not GetRangeSection, it is much higher).
@mangod9
mangod9
commented
Nov 15, 2021

Copy link


Copy Markdown

Member
hi @y-yamshchikov, assume you are still working on this PR feedback?
@mangod9
mangod9
commented
Jan 10, 2022

Copy link


Copy Markdown

Member
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
@y-yamshchikov
y-yamshchikov
commented
Jan 13, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
Dear @mangod9, we are working on new approach granting lockfree reading of RangeSections and still benefiting from binary search capabilities of sorted array, fighting all the contradictions dictating by the multiprocessor environment. We end up with new version of synchronization mechanics and for now working on comprehensive testing environment which grants extensive coverage of all possible synchronization issues. To be presented soon, both the code and the testing model.
@mangod9
mangod9
commented
Jan 13, 2022

Copy link


Copy Markdown

Member
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
@y-yamshchikov
y-yamshchikov
commented
Jan 15, 2022

Copy link


Copy Markdown

ContributorAuthor
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
There is lot of useful feedback conversation still not outdated, I think the change is made in a successive way to prolong current PR.
@mangod9
mangod9
commented
Feb 7, 2022

Copy link


Copy Markdown

Member
I would suggest we close the PR and reopen when more changes are added. This helps to keep the "Open PRs" list manageable.
@mangod9
mangod9
commented
Mar 14, 2022

Copy link


Copy Markdown

Member
Closing for now so it doesnt get flagged in stale PRs. Please reopen when ready to merge again. Thx.
@mangod9mangod9
closed this
Mar 14, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Apr 13, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments

+1 more reviewer

@gbalykovgbalykovgbalykov requested changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Version Bubble implementation flaw: some methods are forced to rejit


5 participants

@y-yamshchikov@alpencolt@jkotas@mangod9@gbalykov

, '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
Search native code in all R2R ni.dll in version bubble - #57277
Closed
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Closed
Search native code in all R2R ni.dll in version bubble#57277
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Conversation
@y-yamshchikov
@y-yamshchikovy-yamshchikov
commented
Aug 12, 2021

Copy link


Copy Markdown

Contributor
This PR fixes part of #44948 and fixes#46160.
This code simply traverses through assemblies in the application

domain. For each assembly (module) it realizes is it Ready To Run and is

it in the same bubble with (does it deliberately bubbling the) module

from which generic function originates. If so, it makes request for code

is Ready To Run and hopes there is some in the module. If the request

succeeds it proceeds with found pointer to the bare native code.
Generic instantiations like System.Collections.Concurrent.ConcurrentDictionary`2[System.Int32,System.__Canon] contained within same version bubble are either a bug in crossgen2 or should be taken care of by having PGO data.

Now such methods use their Ready To Run code.
We have got significant performance gain on startup: 7% average on our representative set on Tizen.
This PR worked out notices in PR below:
#47269

about linear search through the set of RangeSections. In this new PR we propose storing of RangeSections in sorted array (with number of optimizations inspired by prior linked list based solution).
We have extensively tested this PR on armel/Tizen platform so in this case we are confident in reliability and profitability of the solution.
Dear colleagues @jkotas@alpencolt@gbalykov@t-mustafin, please take a look.
@ghostghost
added

area-ReadyToRun

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 12, 2021
@y-yamshchikovy-yamshchikov
changed the title
Partial fix 44948Search native code in all R2R ni.dll in version bubbleAug 12, 2021
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
7 times, most recently
from
c2d2f5d to
91e753aCompareAugust 13, 2021 17:50
gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jaredparjaredpar
mentioned this pull request
Aug 17, 2021

Closed

gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
3 times, most recently
from
da2f1cd to
8dc0aa9CompareSeptember 3, 2021 12:50
@alpencolt
alpencolt
commented
Sep 3, 2021

Copy link


Copy Markdown

@jkotas had you time to check PR?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

This needs comment on what this returns. It does not return a simple index...
@jkotasjkotasSep 3, 2021

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.

Also, can it be in the private section (before public:)?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

In debug/checked builds, we should make the initial size small and make the array grow on element at a time to ensure that the array resize algorithm and the potential race conditions are exercised.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

Why does this need to be volatile?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

I do not think that this needs to store HighAddress. It can just store the LowAddress and the method that is binary searching inside the array can just return the candidate RS index. We can then validate that the candidate RS index actually fits by comparing the address with RangeSection::HighAddress.
@jkotasjkotasSep 3, 2021

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.

Side-effect of this change is going to be that the size of this struct will be power of two that will make the binary search faster.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021

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.

BEGIN_PRESERVE_LAST_ERROR should be before the newly added code.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021•

edited

Loading

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.

It does not look right to create a dummy ExternalMethodFrame just to get the return address.
The fixups from R2R modules should come via ExternalMethodFixupWorker. Can we just use the module that is computed there?
@jkotas
jkotas
commented
Sep 3, 2021

Copy link


Copy Markdown

Member
RangeSections now stored in sorted array
BTW: I do like this change - it should be general performance improvement for any scenario with large number of modules.
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
5 times, most recently
from
3d7cad1 to
fe70768CompareSeptember 16, 2021 11:24
@runfoapprunfoappBot
mentioned this pull request
Oct 13, 2021

Closed

jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

It is important for this path to be lock-free. Adding a lock here is very likely to regress GC stackwalking performance on machines with a lot of cores.
@y-yamshchikovy-yamshchikovOct 19, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

If it is so important, we will investigate another way to isolate readers from writers. We think we could find one with two interchanging arrays.
jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

I am not sure what this is trying to achieve, but it does not look right.
@y-yamshchikovy-yamshchikovOct 19, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We are obliged to ask if the lock has been aquired after NoHostCalls constructor of ReaderLockHolder. NoHostCalls is necessary because of contract requirement from very above on call stack. We have encountered contract violation if we use default constructor (and the source of violation is not GetRangeSection, it is much higher).
@mangod9
mangod9
commented
Nov 15, 2021

Copy link


Copy Markdown

Member
hi @y-yamshchikov, assume you are still working on this PR feedback?
@mangod9
mangod9
commented
Jan 10, 2022

Copy link


Copy Markdown

Member
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
@y-yamshchikov
y-yamshchikov
commented
Jan 13, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
Dear @mangod9, we are working on new approach granting lockfree reading of RangeSections and still benefiting from binary search capabilities of sorted array, fighting all the contradictions dictating by the multiprocessor environment. We end up with new version of synchronization mechanics and for now working on comprehensive testing environment which grants extensive coverage of all possible synchronization issues. To be presented soon, both the code and the testing model.
@mangod9
mangod9
commented
Jan 13, 2022

Copy link


Copy Markdown

Member
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
@y-yamshchikov
y-yamshchikov
commented
Jan 15, 2022

Copy link


Copy Markdown

ContributorAuthor
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
There is lot of useful feedback conversation still not outdated, I think the change is made in a successive way to prolong current PR.
@mangod9
mangod9
commented
Feb 7, 2022

Copy link


Copy Markdown

Member
I would suggest we close the PR and reopen when more changes are added. This helps to keep the "Open PRs" list manageable.
@mangod9
mangod9
commented
Mar 14, 2022

Copy link


Copy Markdown

Member
Closing for now so it doesnt get flagged in stale PRs. Please reopen when ready to merge again. Thx.
@mangod9mangod9
closed this
Mar 14, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Apr 13, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments

+1 more reviewer

@gbalykovgbalykovgbalykov requested changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Version Bubble implementation flaw: some methods are forced to rejit


5 participants

@y-yamshchikov@alpencolt@jkotas@mangod9@gbalykov

, '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
Search native code in all R2R ni.dll in version bubble - #57277
Closed
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Closed
Search native code in all R2R ni.dll in version bubble#57277
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Conversation
@y-yamshchikov
@y-yamshchikovy-yamshchikov
commented
Aug 12, 2021

Copy link


Copy Markdown

Contributor
This PR fixes part of #44948 and fixes#46160.
This code simply traverses through assemblies in the application

domain. For each assembly (module) it realizes is it Ready To Run and is

it in the same bubble with (does it deliberately bubbling the) module

from which generic function originates. If so, it makes request for code

is Ready To Run and hopes there is some in the module. If the request

succeeds it proceeds with found pointer to the bare native code.
Generic instantiations like System.Collections.Concurrent.ConcurrentDictionary`2[System.Int32,System.__Canon] contained within same version bubble are either a bug in crossgen2 or should be taken care of by having PGO data.

Now such methods use their Ready To Run code.
We have got significant performance gain on startup: 7% average on our representative set on Tizen.
This PR worked out notices in PR below:
#47269

about linear search through the set of RangeSections. In this new PR we propose storing of RangeSections in sorted array (with number of optimizations inspired by prior linked list based solution).
We have extensively tested this PR on armel/Tizen platform so in this case we are confident in reliability and profitability of the solution.
Dear colleagues @jkotas@alpencolt@gbalykov@t-mustafin, please take a look.
@ghostghost
added

area-ReadyToRun

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 12, 2021
@y-yamshchikovy-yamshchikov
changed the title
Partial fix 44948Search native code in all R2R ni.dll in version bubbleAug 12, 2021
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
7 times, most recently
from
c2d2f5d to
91e753aCompareAugust 13, 2021 17:50
gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jaredparjaredpar
mentioned this pull request
Aug 17, 2021

Closed

gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
3 times, most recently
from
da2f1cd to
8dc0aa9CompareSeptember 3, 2021 12:50
@alpencolt
alpencolt
commented
Sep 3, 2021

Copy link


Copy Markdown

@jkotas had you time to check PR?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

This needs comment on what this returns. It does not return a simple index...
@jkotasjkotasSep 3, 2021

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.

Also, can it be in the private section (before public:)?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

In debug/checked builds, we should make the initial size small and make the array grow on element at a time to ensure that the array resize algorithm and the potential race conditions are exercised.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

Why does this need to be volatile?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

I do not think that this needs to store HighAddress. It can just store the LowAddress and the method that is binary searching inside the array can just return the candidate RS index. We can then validate that the candidate RS index actually fits by comparing the address with RangeSection::HighAddress.
@jkotasjkotasSep 3, 2021

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.

Side-effect of this change is going to be that the size of this struct will be power of two that will make the binary search faster.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021

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.

BEGIN_PRESERVE_LAST_ERROR should be before the newly added code.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021•

edited

Loading

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.

It does not look right to create a dummy ExternalMethodFrame just to get the return address.
The fixups from R2R modules should come via ExternalMethodFixupWorker. Can we just use the module that is computed there?
@jkotas
jkotas
commented
Sep 3, 2021

Copy link


Copy Markdown

Member
RangeSections now stored in sorted array
BTW: I do like this change - it should be general performance improvement for any scenario with large number of modules.
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
5 times, most recently
from
3d7cad1 to
fe70768CompareSeptember 16, 2021 11:24
@runfoapprunfoappBot
mentioned this pull request
Oct 13, 2021

Closed

jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

It is important for this path to be lock-free. Adding a lock here is very likely to regress GC stackwalking performance on machines with a lot of cores.
@y-yamshchikovy-yamshchikovOct 19, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

If it is so important, we will investigate another way to isolate readers from writers. We think we could find one with two interchanging arrays.
jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

I am not sure what this is trying to achieve, but it does not look right.
@y-yamshchikovy-yamshchikovOct 19, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We are obliged to ask if the lock has been aquired after NoHostCalls constructor of ReaderLockHolder. NoHostCalls is necessary because of contract requirement from very above on call stack. We have encountered contract violation if we use default constructor (and the source of violation is not GetRangeSection, it is much higher).
@mangod9
mangod9
commented
Nov 15, 2021

Copy link


Copy Markdown

Member
hi @y-yamshchikov, assume you are still working on this PR feedback?
@mangod9
mangod9
commented
Jan 10, 2022

Copy link


Copy Markdown

Member
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
@y-yamshchikov
y-yamshchikov
commented
Jan 13, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
Dear @mangod9, we are working on new approach granting lockfree reading of RangeSections and still benefiting from binary search capabilities of sorted array, fighting all the contradictions dictating by the multiprocessor environment. We end up with new version of synchronization mechanics and for now working on comprehensive testing environment which grants extensive coverage of all possible synchronization issues. To be presented soon, both the code and the testing model.
@mangod9
mangod9
commented
Jan 13, 2022

Copy link


Copy Markdown

Member
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
@y-yamshchikov
y-yamshchikov
commented
Jan 15, 2022

Copy link


Copy Markdown

ContributorAuthor
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
There is lot of useful feedback conversation still not outdated, I think the change is made in a successive way to prolong current PR.
@mangod9
mangod9
commented
Feb 7, 2022

Copy link


Copy Markdown

Member
I would suggest we close the PR and reopen when more changes are added. This helps to keep the "Open PRs" list manageable.
@mangod9
mangod9
commented
Mar 14, 2022

Copy link


Copy Markdown

Member
Closing for now so it doesnt get flagged in stale PRs. Please reopen when ready to merge again. Thx.
@mangod9mangod9
closed this
Mar 14, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Apr 13, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments

+1 more reviewer

@gbalykovgbalykovgbalykov requested changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Version Bubble implementation flaw: some methods are forced to rejit


5 participants

@y-yamshchikov@alpencolt@jkotas@mangod9@gbalykov

, '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
Search native code in all R2R ni.dll in version bubble - #57277
Closed
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Closed
Search native code in all R2R ni.dll in version bubble#57277
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Conversation
@y-yamshchikov
@y-yamshchikovy-yamshchikov
commented
Aug 12, 2021

Copy link


Copy Markdown

Contributor
This PR fixes part of #44948 and fixes#46160.
This code simply traverses through assemblies in the application

domain. For each assembly (module) it realizes is it Ready To Run and is

it in the same bubble with (does it deliberately bubbling the) module

from which generic function originates. If so, it makes request for code

is Ready To Run and hopes there is some in the module. If the request

succeeds it proceeds with found pointer to the bare native code.
Generic instantiations like System.Collections.Concurrent.ConcurrentDictionary`2[System.Int32,System.__Canon] contained within same version bubble are either a bug in crossgen2 or should be taken care of by having PGO data.

Now such methods use their Ready To Run code.
We have got significant performance gain on startup: 7% average on our representative set on Tizen.
This PR worked out notices in PR below:
#47269

about linear search through the set of RangeSections. In this new PR we propose storing of RangeSections in sorted array (with number of optimizations inspired by prior linked list based solution).
We have extensively tested this PR on armel/Tizen platform so in this case we are confident in reliability and profitability of the solution.
Dear colleagues @jkotas@alpencolt@gbalykov@t-mustafin, please take a look.
@ghostghost
added

area-ReadyToRun

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 12, 2021
@y-yamshchikovy-yamshchikov
changed the title
Partial fix 44948Search native code in all R2R ni.dll in version bubbleAug 12, 2021
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
7 times, most recently
from
c2d2f5d to
91e753aCompareAugust 13, 2021 17:50
gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jaredparjaredpar
mentioned this pull request
Aug 17, 2021

Closed

gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
3 times, most recently
from
da2f1cd to
8dc0aa9CompareSeptember 3, 2021 12:50
@alpencolt
alpencolt
commented
Sep 3, 2021

Copy link


Copy Markdown

@jkotas had you time to check PR?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

This needs comment on what this returns. It does not return a simple index...
@jkotasjkotasSep 3, 2021

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.

Also, can it be in the private section (before public:)?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

In debug/checked builds, we should make the initial size small and make the array grow on element at a time to ensure that the array resize algorithm and the potential race conditions are exercised.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

Why does this need to be volatile?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

I do not think that this needs to store HighAddress. It can just store the LowAddress and the method that is binary searching inside the array can just return the candidate RS index. We can then validate that the candidate RS index actually fits by comparing the address with RangeSection::HighAddress.
@jkotasjkotasSep 3, 2021

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.

Side-effect of this change is going to be that the size of this struct will be power of two that will make the binary search faster.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021

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.

BEGIN_PRESERVE_LAST_ERROR should be before the newly added code.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021•

edited

Loading

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.

It does not look right to create a dummy ExternalMethodFrame just to get the return address.
The fixups from R2R modules should come via ExternalMethodFixupWorker. Can we just use the module that is computed there?
@jkotas
jkotas
commented
Sep 3, 2021

Copy link


Copy Markdown

Member
RangeSections now stored in sorted array
BTW: I do like this change - it should be general performance improvement for any scenario with large number of modules.
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
5 times, most recently
from
3d7cad1 to
fe70768CompareSeptember 16, 2021 11:24
@runfoapprunfoappBot
mentioned this pull request
Oct 13, 2021

Closed

jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

It is important for this path to be lock-free. Adding a lock here is very likely to regress GC stackwalking performance on machines with a lot of cores.
@y-yamshchikovy-yamshchikovOct 19, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

If it is so important, we will investigate another way to isolate readers from writers. We think we could find one with two interchanging arrays.
jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

I am not sure what this is trying to achieve, but it does not look right.
@y-yamshchikovy-yamshchikovOct 19, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We are obliged to ask if the lock has been aquired after NoHostCalls constructor of ReaderLockHolder. NoHostCalls is necessary because of contract requirement from very above on call stack. We have encountered contract violation if we use default constructor (and the source of violation is not GetRangeSection, it is much higher).
@mangod9
mangod9
commented
Nov 15, 2021

Copy link


Copy Markdown

Member
hi @y-yamshchikov, assume you are still working on this PR feedback?
@mangod9
mangod9
commented
Jan 10, 2022

Copy link


Copy Markdown

Member
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
@y-yamshchikov
y-yamshchikov
commented
Jan 13, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
Dear @mangod9, we are working on new approach granting lockfree reading of RangeSections and still benefiting from binary search capabilities of sorted array, fighting all the contradictions dictating by the multiprocessor environment. We end up with new version of synchronization mechanics and for now working on comprehensive testing environment which grants extensive coverage of all possible synchronization issues. To be presented soon, both the code and the testing model.
@mangod9
mangod9
commented
Jan 13, 2022

Copy link


Copy Markdown

Member
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
@y-yamshchikov
y-yamshchikov
commented
Jan 15, 2022

Copy link


Copy Markdown

ContributorAuthor
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
There is lot of useful feedback conversation still not outdated, I think the change is made in a successive way to prolong current PR.
@mangod9
mangod9
commented
Feb 7, 2022

Copy link


Copy Markdown

Member
I would suggest we close the PR and reopen when more changes are added. This helps to keep the "Open PRs" list manageable.
@mangod9
mangod9
commented
Mar 14, 2022

Copy link


Copy Markdown

Member
Closing for now so it doesnt get flagged in stale PRs. Please reopen when ready to merge again. Thx.
@mangod9mangod9
closed this
Mar 14, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Apr 13, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments

+1 more reviewer

@gbalykovgbalykovgbalykov requested changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Version Bubble implementation flaw: some methods are forced to rejit


5 participants

@y-yamshchikov@alpencolt@jkotas@mangod9@gbalykov

, '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
Search native code in all R2R ni.dll in version bubble - #57277
Closed
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Closed
Search native code in all R2R ni.dll in version bubble#57277
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Conversation
@y-yamshchikov
@y-yamshchikovy-yamshchikov
commented
Aug 12, 2021

Copy link


Copy Markdown

Contributor
This PR fixes part of #44948 and fixes#46160.
This code simply traverses through assemblies in the application

domain. For each assembly (module) it realizes is it Ready To Run and is

it in the same bubble with (does it deliberately bubbling the) module

from which generic function originates. If so, it makes request for code

is Ready To Run and hopes there is some in the module. If the request

succeeds it proceeds with found pointer to the bare native code.
Generic instantiations like System.Collections.Concurrent.ConcurrentDictionary`2[System.Int32,System.__Canon] contained within same version bubble are either a bug in crossgen2 or should be taken care of by having PGO data.

Now such methods use their Ready To Run code.
We have got significant performance gain on startup: 7% average on our representative set on Tizen.
This PR worked out notices in PR below:
#47269

about linear search through the set of RangeSections. In this new PR we propose storing of RangeSections in sorted array (with number of optimizations inspired by prior linked list based solution).
We have extensively tested this PR on armel/Tizen platform so in this case we are confident in reliability and profitability of the solution.
Dear colleagues @jkotas@alpencolt@gbalykov@t-mustafin, please take a look.
@ghostghost
added

area-ReadyToRun

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 12, 2021
@y-yamshchikovy-yamshchikov
changed the title
Partial fix 44948Search native code in all R2R ni.dll in version bubbleAug 12, 2021
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
7 times, most recently
from
c2d2f5d to
91e753aCompareAugust 13, 2021 17:50
gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jaredparjaredpar
mentioned this pull request
Aug 17, 2021

Closed

gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
3 times, most recently
from
da2f1cd to
8dc0aa9CompareSeptember 3, 2021 12:50
@alpencolt
alpencolt
commented
Sep 3, 2021

Copy link


Copy Markdown

@jkotas had you time to check PR?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

This needs comment on what this returns. It does not return a simple index...
@jkotasjkotasSep 3, 2021

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.

Also, can it be in the private section (before public:)?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

In debug/checked builds, we should make the initial size small and make the array grow on element at a time to ensure that the array resize algorithm and the potential race conditions are exercised.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

Why does this need to be volatile?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

I do not think that this needs to store HighAddress. It can just store the LowAddress and the method that is binary searching inside the array can just return the candidate RS index. We can then validate that the candidate RS index actually fits by comparing the address with RangeSection::HighAddress.
@jkotasjkotasSep 3, 2021

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.

Side-effect of this change is going to be that the size of this struct will be power of two that will make the binary search faster.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021

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.

BEGIN_PRESERVE_LAST_ERROR should be before the newly added code.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021•

edited

Loading

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.

It does not look right to create a dummy ExternalMethodFrame just to get the return address.
The fixups from R2R modules should come via ExternalMethodFixupWorker. Can we just use the module that is computed there?
@jkotas
jkotas
commented
Sep 3, 2021

Copy link


Copy Markdown

Member
RangeSections now stored in sorted array
BTW: I do like this change - it should be general performance improvement for any scenario with large number of modules.
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
5 times, most recently
from
3d7cad1 to
fe70768CompareSeptember 16, 2021 11:24
@runfoapprunfoappBot
mentioned this pull request
Oct 13, 2021

Closed

jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

It is important for this path to be lock-free. Adding a lock here is very likely to regress GC stackwalking performance on machines with a lot of cores.
@y-yamshchikovy-yamshchikovOct 19, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

If it is so important, we will investigate another way to isolate readers from writers. We think we could find one with two interchanging arrays.
jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

I am not sure what this is trying to achieve, but it does not look right.
@y-yamshchikovy-yamshchikovOct 19, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We are obliged to ask if the lock has been aquired after NoHostCalls constructor of ReaderLockHolder. NoHostCalls is necessary because of contract requirement from very above on call stack. We have encountered contract violation if we use default constructor (and the source of violation is not GetRangeSection, it is much higher).
@mangod9
mangod9
commented
Nov 15, 2021

Copy link


Copy Markdown

Member
hi @y-yamshchikov, assume you are still working on this PR feedback?
@mangod9
mangod9
commented
Jan 10, 2022

Copy link


Copy Markdown

Member
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
@y-yamshchikov
y-yamshchikov
commented
Jan 13, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
Dear @mangod9, we are working on new approach granting lockfree reading of RangeSections and still benefiting from binary search capabilities of sorted array, fighting all the contradictions dictating by the multiprocessor environment. We end up with new version of synchronization mechanics and for now working on comprehensive testing environment which grants extensive coverage of all possible synchronization issues. To be presented soon, both the code and the testing model.
@mangod9
mangod9
commented
Jan 13, 2022

Copy link


Copy Markdown

Member
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
@y-yamshchikov
y-yamshchikov
commented
Jan 15, 2022

Copy link


Copy Markdown

ContributorAuthor
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
There is lot of useful feedback conversation still not outdated, I think the change is made in a successive way to prolong current PR.
@mangod9
mangod9
commented
Feb 7, 2022

Copy link


Copy Markdown

Member
I would suggest we close the PR and reopen when more changes are added. This helps to keep the "Open PRs" list manageable.
@mangod9
mangod9
commented
Mar 14, 2022

Copy link


Copy Markdown

Member
Closing for now so it doesnt get flagged in stale PRs. Please reopen when ready to merge again. Thx.
@mangod9mangod9
closed this
Mar 14, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Apr 13, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments

+1 more reviewer

@gbalykovgbalykovgbalykov requested changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Version Bubble implementation flaw: some methods are forced to rejit


5 participants

@y-yamshchikov@alpencolt@jkotas@mangod9@gbalykov

, '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
Search native code in all R2R ni.dll in version bubble - #57277
Closed
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Closed
Search native code in all R2R ni.dll in version bubble#57277
y-yamshchikov wants to merge 15 commits into
dotnet:mainfrom 
y-yamshchikov:partial-fix-44948
Conversation
@y-yamshchikov
@y-yamshchikovy-yamshchikov
commented
Aug 12, 2021

Copy link


Copy Markdown

Contributor
This PR fixes part of #44948 and fixes#46160.
This code simply traverses through assemblies in the application

domain. For each assembly (module) it realizes is it Ready To Run and is

it in the same bubble with (does it deliberately bubbling the) module

from which generic function originates. If so, it makes request for code

is Ready To Run and hopes there is some in the module. If the request

succeeds it proceeds with found pointer to the bare native code.
Generic instantiations like System.Collections.Concurrent.ConcurrentDictionary`2[System.Int32,System.__Canon] contained within same version bubble are either a bug in crossgen2 or should be taken care of by having PGO data.

Now such methods use their Ready To Run code.
We have got significant performance gain on startup: 7% average on our representative set on Tizen.
This PR worked out notices in PR below:
#47269

about linear search through the set of RangeSections. In this new PR we propose storing of RangeSections in sorted array (with number of optimizations inspired by prior linked list based solution).
We have extensively tested this PR on armel/Tizen platform so in this case we are confident in reliability and profitability of the solution.
Dear colleagues @jkotas@alpencolt@gbalykov@t-mustafin, please take a look.
@ghostghost
added

area-ReadyToRun

community-contribution
Indicates that the PR has been added by a community member
labels
Aug 12, 2021
@y-yamshchikovy-yamshchikov
changed the title
Partial fix 44948Search native code in all R2R ni.dll in version bubbleAug 12, 2021
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
7 times, most recently
from
c2d2f5d to
91e753aCompareAugust 13, 2021 17:50
gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.h
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jaredparjaredpar
mentioned this pull request
Aug 17, 2021

Closed

gbalykov
gbalykov
suggested changes
Aug 17, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
3 times, most recently
from
da2f1cd to
8dc0aa9CompareSeptember 3, 2021 12:50
@alpencolt
alpencolt
commented
Sep 3, 2021

Copy link


Copy Markdown

@jkotas had you time to check PR?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

This needs comment on what this returns. It does not return a simple index...
@jkotasjkotasSep 3, 2021

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.

Also, can it be in the private section (before public:)?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

In debug/checked builds, we should make the initial size small and make the array grow on element at a time to ensure that the array resize algorithm and the potential race conditions are exercised.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

Why does this need to be volatile?
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.h
Outdated

@jkotasjkotasSep 3, 2021

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.

I do not think that this needs to store HighAddress. It can just store the LowAddress and the method that is binary searching inside the array can just return the candidate RS index. We can then validate that the candidate RS index actually fits by comparing the address with RangeSection::HighAddress.
@jkotasjkotasSep 3, 2021

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.

Side-effect of this change is going to be that the size of this struct will be power of two that will make the binary search faster.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021

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.

BEGIN_PRESERVE_LAST_ERROR should be before the newly added code.
jkotas
jkotas
reviewed
Sep 3, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/prestub.cpp
Outdated

@jkotasjkotasSep 3, 2021•

edited

Loading

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.

It does not look right to create a dummy ExternalMethodFrame just to get the return address.
The fixups from R2R modules should come via ExternalMethodFixupWorker. Can we just use the module that is computed there?
@jkotas
jkotas
commented
Sep 3, 2021

Copy link


Copy Markdown

Member
RangeSections now stored in sorted array
BTW: I do like this change - it should be general performance improvement for any scenario with large number of modules.
@y-yamshchikov
y-yamshchikovforce-pushed
the
partial-fix-44948
branch
5 times, most recently
from
3d7cad1 to
fe70768CompareSeptember 16, 2021 11:24
@runfoapprunfoappBot
mentioned this pull request
Oct 13, 2021

Closed

jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

It is important for this path to be lock-free. Adding a lock here is very likely to regress GC stackwalking performance on machines with a lot of cores.
@y-yamshchikovy-yamshchikovOct 19, 2021

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

If it is so important, we will investigate another way to isolate readers from writers. We think we could find one with two interchanging arrays.
jkotas
jkotas
reviewed
Oct 18, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp
Outdated

@jkotasjkotasOct 18, 2021

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.

I am not sure what this is trying to achieve, but it does not look right.
@y-yamshchikovy-yamshchikovOct 19, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We are obliged to ask if the lock has been aquired after NoHostCalls constructor of ReaderLockHolder. NoHostCalls is necessary because of contract requirement from very above on call stack. We have encountered contract violation if we use default constructor (and the source of violation is not GetRangeSection, it is much higher).
@mangod9
mangod9
commented
Nov 15, 2021

Copy link


Copy Markdown

Member
hi @y-yamshchikov, assume you are still working on this PR feedback?
@mangod9
mangod9
commented
Jan 10, 2022

Copy link


Copy Markdown

Member
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
@y-yamshchikov
y-yamshchikov
commented
Jan 13, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
hi @y-yamshchikov, checking if this PR still needs to be kept open since it hasnt been updated in a couple of months. Thx
Dear @mangod9, we are working on new approach granting lockfree reading of RangeSections and still benefiting from binary search capabilities of sorted array, fighting all the contradictions dictating by the multiprocessor environment. We end up with new version of synchronization mechanics and for now working on comprehensive testing environment which grants extensive coverage of all possible synchronization issues. To be presented soon, both the code and the testing model.
@mangod9
mangod9
commented
Jan 13, 2022

Copy link


Copy Markdown

Member
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
@y-yamshchikov
y-yamshchikov
commented
Jan 15, 2022

Copy link


Copy Markdown

ContributorAuthor
thanks for the update, should we close this PR for now and open a new one once ready? Thx!
There is lot of useful feedback conversation still not outdated, I think the change is made in a successive way to prolong current PR.
@mangod9
mangod9
commented
Feb 7, 2022

Copy link


Copy Markdown

Member
I would suggest we close the PR and reopen when more changes are added. This helps to keep the "Open PRs" list manageable.
@mangod9
mangod9
commented
Mar 14, 2022

Copy link


Copy Markdown

Member
Closing for now so it doesnt get flagged in stale PRs. Please reopen when ready to merge again. Thx.
@mangod9mangod9
closed this
Mar 14, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Apr 13, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments

+1 more reviewer

@gbalykovgbalykovgbalykov requested changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Version Bubble implementation flaw: some methods are forced to rejit


5 participants

@y-yamshchikov@alpencolt@jkotas@mangod9@gbalykov