Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Port yield normalization from CoreCLR to Native AOT#103675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
eduardo-vp merged 23 commits intodotnet:mainfromeduardo-vp:port-yield-norm-to-aot
Jul 17, 2024

Conversation

eduardo-vp
Copy link
Member

@eduardo-vpeduardo-vp commentedJun 18, 2024
edited
Loading

Porting the current way yield normalization is done to Native AOT.

The CoreCLR implementation was moved to src/coreclr/vm/yieldprocessornormalizedshared.cpp.

Both the CoreCLR file (src/coreclr/vm/yieldprocessornormalized.cpp) and the Native AOT file (src/coreclr/nativeaot/Runtime/yieldprocessornormalized.cpp) now share the same implementation.

@ghostghost added the needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners labelJun 18, 2024
@eduardo-vpeduardo-vp added area-System.Threading and removed needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners labelsJun 18, 2024
@dotnet-policy-serviceDotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area:@mangod9
See info inarea-owners.md if you want to be subscribed.

@@ -46,9 +46,6 @@ uint32_t WINAPI FinalizerStart(void* pContext)

g_pFinalizerThread = PTR_Thread(pThread);

// We have some time until the first finalization request - use the time to calibrate normalized waits.
EnsureYieldProcessorNormalizedInitialized();
Copy link
Member

Choose a reason for hiding this comment

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

How is the measurement going to be triggered when this is deleted?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I'm still trying to figure this out, I'm not very familiar with Native AOT in general so I'd appreciate any suggestions

Copy link
Member

Choose a reason for hiding this comment

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

It looks like we would need to callYieldProcessorNormalization::PerformMeasurement() from here or add aEnsureYieldProcessorNormalizedInitialized() entry point to the new code that simply callsYieldProcessorNormalization::PerformMeasurement()

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Do you happen to know if this function is called every ~4 seconds or faster than that? Currently we letYieldProcessorNormalization::PerformMeasurement() run every ~4 s so if that's the case, I believe we may add here the same call as in CoreCLR

if (YieldProcessorNormalization::IsMeasurementScheduled())    {GCX_PREEMP();YieldProcessorNormalization::PerformMeasurement();    }

Copy link
Member

Choose a reason for hiding this comment

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

FinalizerStart function is called once per process. It is equivalent ofFinalizerThreadStart function in regular CoreCLR.

I think you want to follow the same structure as in regular CoreCLR: Trigger the measurement from ScheduleMeasurementIfNecessary by callingRhEnableFinalization (it is equivalent ofFinalizerThread::EnableFinalization in regular CoreCLR) and then add the measurement to loop inProcessFinalizers().

Copy link
Member

@VSadovVSadovJun 24, 2024
edited
Loading

Choose a reason for hiding this comment

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

Do you happen to know if this function is called every ~4 seconds or faster than that?

I am not sure. The whole deal with measuring duration of something that is proportional to CPU cycle is not very precise, since the CPU cycle can change drastically and many times per second and will be different for every core. Unless machine is configured into HighPerformance power plan, every measurement is a bit of a coin toss and will produce the same result with the same error margins.

The main purpose of calibration is to continue using historically hard-coded spin counts in numerous places where we spinwait while allowing that to work on systems with vastly different pause durations (i.e. on post-skylake intel CPUs pause takes ~140 cycles, pre-skylake is about ~10 cycles). For such purpose the callibration is precise enough.

I am not sure about the value of redoing the measurement over and over.
Perhaps to support scenarios where a VM is migrated between pre/post skylake machines.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can add a periodic call PerformMeasurement in NativeAOT and see what happens.

My guess - nothing will change, just a bit more time spent in PerformMeasurement.

Copy link
Member

@VSadovVSadovJun 24, 2024
edited
Loading

Choose a reason for hiding this comment

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

There is value in having the same behavior though.
If the re-measuring (or the whole calibration deal) could be somehow avoided or improved, it would make sense to do for both runtimes.

jkotas reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

IIRC there's a good reason to keep re-doing measurements, so probably keeping this behaviour in Native AOT would be better, I believe@kouvel or@mangod9 may elaborate better

Copy link
Contributor

Choose a reason for hiding this comment

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

The measurements done are very short and can be perturbed by CPU activity, the rolling min helps to stabilize it over time.

@jkotasjkotas requested review fromVSadov andkouvelJune 22, 2024 01:34
@eduardo-vpeduardo-vpforce-pushed theport-yield-norm-to-aot branch fromaf5ceaa to6519b0bCompareJuly 2, 2024 18:48
@eduardo-vpeduardo-vp marked this pull request as ready for reviewJuly 2, 2024 23:09
Eduardo Manuel Velarde Polar added2 commitsJuly 2, 2024 17:04
Copy link
Contributor

@kouvelkouvel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jkotas
Copy link
Member

What kind of testing you have done on the change to validate that it works as expected? Do we expect improvements in any perf benchmarks?

@kouvel
Copy link
Contributor

kouvel commentedJul 4, 2024
edited
Loading

I don't think there would be any changes to benchmarks. I would expect that the CPU time spent during startup in the measurements would be a lot less (the new scheme measures lazily, and in narrower windows), that's about it. It would be good to measure that.

@eduardo-vp
Copy link
MemberAuthor

I tested the following snippet and I checked that the 8 initial measurements were done and subsequent measurements every ~4 seconds were done as well.

usingSystem;usingSystem.Threading;intminutesToSpin=10;intstartTicks=Environment.TickCount;while(Environment.TickCount-startTicks<minutesToSpin*60*1000){Thread.SpinWait(1000);Thread.Sleep(2000);}

image

kouvel reacted with thumbs up emoji

@eduardo-vpeduardo-vp merged commitd35f302 intodotnet:mainJul 17, 2024
87 of 89 checks passed
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull requestJul 26, 2024
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull requestJul 26, 2024
eduardo-vp pushed a commit to eduardo-vp/runtime that referenced this pull requestAug 13, 2024
eduardo-vp pushed a commit to eduardo-vp/runtime that referenced this pull requestAug 14, 2024
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsAug 17, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@kouvelkouvelkouvel approved these changes

@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

@jkotasjkotasAwaiting requested review from jkotas

@VSadovVSadovAwaiting requested review from VSadov

Assignees

@eduardo-vpeduardo-vp

Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@eduardo-vp@jkotas@kouvel@VSadov

[8]ページ先頭

©2009-2025 Movatter.jp