This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Porting desktop changes to core #21523
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jkotas
reviewed
Dec 13, 2018
| GC_NOTRIGGER; | ||
| } CONTRACTL_END; | ||
|
|
||
| if (strcmp(key, "GCSegmentSize") == 0) |
Member
There was a problem hiding this comment.
Do we really need to special case these 3 config settings for CoreCLR here?
Member
Author
There was a problem hiding this comment.
the idea was we didn't want to do the rest at all if it's one of the ones the VM already did the work to parse and has the info for.
jkotas
reviewed
Dec 13, 2018
src/gc/gc.cpp
Outdated
| affinity.Group = GCThreadAffinity::None; | ||
| affinity.Processor = GCThreadAffinity::None; | ||
|
|
||
| #if !defined(FEATURE_REDHAWK) && !defined(FEATURE_CORECLR) |
Member
There was a problem hiding this comment.
This should not be undef ifdef.
Also, note that there is same piece of code in gc_thread_stub. It does not sound right to have it in both places.
Member
Author
There was a problem hiding this comment.
oh.....that's totally a porting error. thanks for spotting it!
jkotas
reviewed
Dec 13, 2018
src/gc/gc.cpp
Outdated
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
| // |
jkotas
reviewed
Dec 13, 2018
src/gc/gc.cpp
Outdated
| // | ||
| // GC automatically manages memory allocated by managed code. | ||
| // The design doc for GC can be found at | ||
| // file:../../doc/BookOfTheRuntime/GC/GCDesign.doc |
janvorli
reviewed
Dec 13, 2018
src/gc/gc.cpp
Outdated
| @@ -486,7 +533,7 @@ void log_va_msg(const char *fmt, va_list args) | |||
| int pid_len = sprintf_s (&pBuffer[buffer_start], BUFFERSIZE - buffer_start, "[%5d]", (uint32_t)GCToOSInterface::GetCurrentThreadIdForLogging()); | |||
| buffer_start += pid_len; | |||
| memset(&pBuffer[buffer_start], '-', BUFFERSIZE - buffer_start); | |||
| int msg_len = _vsnprintf_s(&pBuffer[buffer_start], BUFFERSIZE - buffer_start, _TRUNCATE, fmt, args ); | |||
| int msg_len = _vsnprintf(&pBuffer[buffer_start], BUFFERSIZE - buffer_start, fmt, args ); | |||
Member
There was a problem hiding this comment.
Could you please revert this change? I've changed that last year when I was removing all usages of unsafe printf functions from the runtime and GC.
Member
Author
There was a problem hiding this comment.
porting error, thanks for spotting!
src/gc/gc.cpp
Outdated
| @@ -3015,12 +3085,13 @@ void gc_heap::fire_pevents() | |||
| settings.record (&gc_data_global); | |||
| gc_data_global.print(); | |||
|
|
|||
| FIRE_EVENT(GCGlobalHeapHistory_V2, gc_data_global.final_youngest_desired, | |||
| FireEtwGCGlobalHeapHistory_V2(gc_data_global.final_youngest_desired, | |||
Member
There was a problem hiding this comment.
What is the reason for changing from FIRE_EVENT(GCGlobalHeapHistory_V2 to FireEtwGCGlobalHeapHistory_V2 ?
Member
Author
There was a problem hiding this comment.
all FireEtw* (actually all except one...we should fix that) were changed to FIRE_EVENT for LocalGC because we wanna check if it's enabled on the GC side
src/gc/gc.cpp
Outdated
| @@ -25726,7 +26050,7 @@ void gc_heap::background_mark_phase () | |||
| #endif //MULTIPLE_HEAPS | |||
| } | |||
|
|
|||
| disable_preemptive (true); | |||
| disable_preemptive (TRUE); | |||
Member
There was a problem hiding this comment.
the disable_preemptive has a "bool" parameter type, so this looks like an unintended change (the same few lines below)
src/gc/gc.cpp
Outdated
| @@ -27700,7 +28017,8 @@ void gc_heap::mark_through_cards_for_segments (card_fn fn, BOOL relocating) | |||
| size_t n_eph = 0; | |||
| size_t n_gen = 0; | |||
| size_t n_card_set = 0; | |||
| uint8_t* nhigh = (relocating ? heap_segment_plan_allocated (ephemeral_heap_segment) : high); | |||
| uint8_t* nhigh = (relocating ? | |||
| process_mask = pmask; | ||
|
|
||
| unsigned int set_bits_in_pmask = 0; | ||
| while (pmask) |
Member
There was a problem hiding this comment.
A nit - an alternative if-less way to get number of set bits would be
while (pmask != 0)
{
set_bits_in_pmask++;
pmask &= (pmask - 1);
}
Member
Author
There was a problem hiding this comment.
yep that's more efficient - but it makes no actual difference cause other calls will dominate so I'll leave it the same as on desktop (code is a bit easier to read).
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
| // Copyright (c) Microsoft. All rights reserved. |
Member
There was a problem hiding this comment.
The licence header should stay as it was.
Member
Author
There was a problem hiding this comment.
yep already pointed out by JanK
src/gc/gcpriv.h
Outdated
|
|
||
| PER_HEAP | ||
| uint8_t* oldest_pinned_plug; | ||
| uint8_t* oldest_pinned_plug; |
Member
There was a problem hiding this comment.
Looks like unintentional change.
Member
Author
|
this addresses all feedback above + a bunch of things I found while going through the changes + correct SetYieldProcessorScalingFactor for core. |
Maoni0
changed the title
Member
Author
|
@dotnet_bot test Windows_NT Checked longgc |
Member
|
Member
Author
|
thanks Jan! msvc compiler does not complain about this. ahh well. fixed and resubmitted. |
Member
Author
|
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test |
Member
Author
|
@dotnet-bot test CentOS7.1 x64 Debug Innerloop Build |
Member
Author
|
@dotnet-bot test Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0) |
1 similar comment
Member
Author
|
@dotnet-bot test Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0) |
Member
|
@dotnet-bot test this please |
Member
|
@dotnet-bot test Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0) |
Member
Author
|
@dotnet_bot test Windows_NT Release longgc |
Member
|
@dotnet-bot test Windows_NT x86 Checked Innerloop Build and Test |
Member
|
@dotnet_bot test Windows_NT Checked longgc |
Member
|
@dotnet-bot help |
|
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Member
|
@dotnet-bot test Windows_NT x64 Release longgc |
Member
|
@dotnet_bot test Windows_NT x64 Checked longgc |
1 similar comment
Member
|
@dotnet_bot test Windows_NT x64 Checked longgc |
Member
Author
|
@dotnet_bot test Windows_NT x64 Checked longgc |
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
+alloc lock split into SOH and LOH +provisional mode to fix too many gen2 GCs triggered in low mem situation when the heap has heavy pinning fragmentation +better free list usage +premature OOM fixes +3 new configs: GCHeapAffinitizeMask, GCHighMemPercent, GCLOHThreshold (will be documented) YieldProcessor scaling factor is different on core due to the different implementation on core. Commit migrated from dotnet/coreclr@aa13ca9
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this includes the following -
+alloc lock split into SOH and LOH
+provisional mode to fix too many gen2 GCs triggered in low mem situation when the heap has heavy pinning fragmentation
+better free list usage
+premature OOM fixes
+3 new configs: GCHeapAffinitizeMask, GCHighMemPercent, GCLOHThreshold (will be documented)
I still have one perf todo for the YieldProcessor scaling factor that I have to test as the desktop and coreclr implementations are different. I'll get to it tomorrow.