statically linking GC PAL on linux by mangod9 · Pull Request #76985 · dotnet/runtime

mangod9 · 2022-10-13T03:49:09Z

The GC PAL will be used for both coreclr and standalone GC on linux. FIxes#72684.

Have validated that it reduces the working set for unbounded memory limits by ~70mb for a vanilla webapi app.

The GC PAL will be used for both coreclr and standalone GC on linux

ghost · 2022-10-13T03:49:22Z

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

The GC PAL will be used for both coreclr and standalone GC on linux. FIxes #72684.

Have validated that it reduces the working set for unbounded memory limits by ~70mb for a vanilla webapi app.

Author:	mangod9
Assignees:	-
Labels:	`NO-MERGE`, `area-GC-coreclr`
Milestone:	-

janvorli · 2022-10-13T12:02:11Z

@mangod9 I would make the change for all OSes, not just Unix.

mangod9 · 2022-10-15T05:53:58Z

 goto CLEANUP15;
 }

- if (FALSE == NUMASupportInitialize())


assume its fine to remove the Numa initialization from the pal, since its now being handled with the gc_unix pal.

jkotas · 2022-10-15T18:44:21Z

+#ifdef HOST_WINDOWS
+
 //******************************************************************************
 // NumaNodeInfo


Delete the NumaNodeInfo class?

yeah guess there is more cleanup required here, will do as part of a separate PR.

I see that you are keeping the VM version of the current PAL for Windows under ifdefs. Is there a problem with using the GC PAL on Windows?

there shouldnt be an issue with moving windows too, just needs a some more wrangling. Also need to check with @Maoni0 that all numa stuff is handled appropriately in gcenv.windows

mangod9 · 2022-10-19T22:42:47Z

/azp run runtime

azure-pipelines · 2022-10-19T22:43:22Z

Azure Pipelines successfully started running 1 pipeline(s).

Maoni0

LGTM

EgorBo · 2022-10-27T19:34:48Z

@mangod9 Could this decrease working set in TE benchmarks (only on Linux)?

e.g.

it's something from this range: ee41716...7b5ab35

mangod9 · 2022-10-27T19:38:55Z

yeah, that was one of the main reasons for the change.

* statically linking GC PAL The GC PAL will be used for both coreclr and standalone GC on linux * fixing arm64 and nativeaot build breaks * macos build break and reducing renaming. * trying to remove numa support from PAL * one more rename to resolve MacOS break * delete pal numa code. * Adding missing madvise in GC PAL * added missing MADV_DONTDUMP calls. * CR feedback * undo (long long) cast in GetMemoryStatus * only invoke madvise on success.

sebastienros · 2023-02-13T18:42:14Z

Is it possible that it also improved the startup time? Many benchmarks saw an improvement on the same changeset containing this PR.

jkotas · 2023-02-13T18:47:36Z

Is it possible that it also improved the startup time?

Yes, the PAL did a bunch of extra useless work before this change.

statically linking GC PAL
41710cb
The GC PAL will be used for both coreclr and standalone GC on linux

mangod9 added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-GC-coreclr labels Oct 13, 2022

ghost assigned mangod9 Oct 13, 2022

mangod9 commented Oct 13, 2022
View reviewed changes

Comment threadsrc/coreclr/gc/unix/gcenv.unix.cpp Outdated

fixing arm64 and nativeaot build breaks
61426d7

build-analysisBot mentioned this pull request Oct 13, 2022
Assertion failed: (*card_word)==0 in DynamicGenerics tests #76801
Closed

jkotas reviewed Oct 13, 2022
View reviewed changes

Comment threadsrc/coreclr/gc/unix/gcenv.unix.cpp Outdated

macos build break and reducing renaming.
225f725

jkotas reviewed Oct 13, 2022
View reviewed changes

Comment threadsrc/coreclr/gc/unix/gcenv.unix.cpp Outdated

jkotas reviewed Oct 13, 2022
View reviewed changes

Comment threadsrc/coreclr/gc/unix/gcenv.unix.cpp Outdated

This was referenced Oct 14, 2022
Tracking Nuget 429s dotnet/arcade#10885
Closed
Tracking issue for CI build timeouts #76454
Closed
Nuget restore error caused by 503s dotnet/arcade#11239
Closed

mangod9 added 2 commits October 14, 2022 20:05

trying to remove numa support from PAL
4c54b0a

one more rename to resolve MacOS break
c4ac1e7

mangod9 commented Oct 15, 2022
View reviewed changes

jkotas reviewed Oct 15, 2022
View reviewed changes

Comment threadsrc/coreclr/pal/src/CMakeLists.txt

jkotas reviewed Oct 15, 2022
View reviewed changes

delete pal numa code.
64af4b7

mangod9 changed the title ~~[WIP] statically linking GC PAL~~statically linking GC PALOct 17, 2022

mangod9 requested a review from Maoni0 October 17, 2022 17:12

mangod9 removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 17, 2022

mangod9 changed the title ~~statically linking GC PAL~~statically linking GC PAL on linuxOct 17, 2022

mangod9 added 3 commits October 18, 2022 13:35

Merge branch 'main' into use_gc_pal
0e90a82

Adding missing madvise in GC PAL
a585fc3

added missing MADV_DONTDUMP calls.
4c53ff8

mangod9 commented Oct 19, 2022
View reviewed changes

Comment threadsrc/coreclr/gc/unix/gcenv.unix.cpp Outdated

mangod9 mentioned this pull request Oct 19, 2022
make GC's PAL implementation include the right dump info #77184
Closed

jkotas reviewed Oct 19, 2022
View reviewed changes

Comment threadsrc/coreclr/gc/unix/gcenv.unix.cpp

jkotas reviewed Oct 19, 2022
View reviewed changes

Comment threadsrc/coreclr/gc/unix/gcenv.unix.cpp Outdated

janvorli force-pushed the use_gc_pal branch 2 times, most recently from 263a2f0 to dccc8c6Compare October 19, 2022 22:09

mangod9 force-pushed the use_gc_pal branch from dccc8c6 to 4c53ff8Compare October 19, 2022 22:26

mangod9 added 3 commits October 19, 2022 17:43

CR feedback
6d441b7

Merge branch 'main' into use_gc_pal
d6450eb

undo (long long) cast in GetMemoryStatus
71a5896

Maoni0 reviewed Oct 21, 2022
View reviewed changes

Comment threadsrc/coreclr/gc/unix/gcenv.unix.cpp Outdated

Maoni0 reviewed Oct 21, 2022
View reviewed changes

Comment threadsrc/coreclr/gc/unix/gcenv.unix.cpp Outdated

Maoni0 reviewed Oct 21, 2022
View reviewed changes

Comment threadsrc/coreclr/gc/unix/gcenv.unix.cpp Outdated

only invoke madvise on success.
b6d04a1

mangod9 requested a review from Maoni0 October 25, 2022 18:02

Maoni0 approved these changes Oct 25, 2022
View reviewed changes

mangod9 merged commit c4341d4 into dotnet:mainOct 25, 2022

EgorBo mentioned this pull request Oct 27, 2022
PGO: Add new tiers #70941
Merged

This was referenced Nov 1, 2022
[Perf] Linux/x64: 3 Improvements on 10/25/2022 10:16:52 PM dotnet/perf-autofiling-issues#9333
Closed
Regressions in ICU globalization microbenchmarks #77730
Closed

ghost locked as resolved and limited conversation to collaborators Nov 27, 2022

Uh oh!

Conversation

mangod9 commented Oct 13, 2022

Uh oh!

ghost commented Oct 13, 2022

Uh oh!

Uh oh!

janvorli commented Oct 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mangod9Oct 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkotasOct 15, 2022

Choose a reason for hiding this comment

Uh oh!

mangod9Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

jkotasOct 17, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mangod9Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mangod9 commented Oct 19, 2022

Uh oh!

azure-pipelinesBot commented Oct 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Maoni0 left a comment

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Oct 27, 2022

Uh oh!

mangod9 commented Oct 27, 2022

Uh oh!

sebastienros commented Feb 13, 2023

Uh oh!

jkotas commented Feb 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jkotasOct 17, 2022•
edited
Loading