Host with coreclr linked in #36847

VSadov · 2020-05-21T22:12:18Z

Related to: #37119

This change results in singlefilehost to embed coreclr and JIT code.

Notes:

this is not yet enabled on Windows.
Native libraries like System.IO.Compression.Native.a are not linked in as a part of this change. Will be done separately.

ghost · 2020-05-21T22:12:23Z

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

src/coreclr/src/pal/src/loader/module.cpp

src/coreclr/src/debug/ee/wks/CMakeLists.txt

mjsabby · 2020-06-03T03:53:17Z

The superhost doc says due to some debugging issues there won't be a single static lib for coreclr on windows. For anyone not wanting to debug will there be a way to achieve this functionality?

VSadov · 2020-06-03T04:22:46Z

@swaroop-sridhar would know better if there are other issues besides debugging on Windows.

…one coreclr, we do not need EMBEDDED_RUNTIME marker.

VSadov · 2020-06-25T19:42:10Z

Rebased onto most recent master.

janvorli · 2020-06-26T11:59:12Z

src/coreclr/src/pal/src/loader/module.cpp

+
+    // Getting nullptr as name indicates redirection to current library
+    if (libraryNameOrPath == nullptr)
+        return true;


Ah, right, if those libs get linked into the shared coreclr statically too, we won't need any changes here.

janvorli · 2020-06-26T12:06:55Z

src/installer/corehost/cli/apphost/static/CMakeLists.txt

@@ -56,8 +56,91 @@ if (CLR_CMAKE_TARGET_WIN32 AND (CLR_CMAKE_TARGET_ARCH_ARM OR CLR_CMAKE_TARGET_AR
    target_link_libraries(singlefilehost Advapi32.lib shell32.lib)
 endif()

+# Path like: artifacts/bin/coreclr/Windows_NT.x64.Release/lib  or
+#            /root/runtime/artifacts/transport/coreclr/lib
+set(CORECLR_STATIC_LIB_LOCATION "$ENV{__CoreClrArtifacts}lib")


A nit - can we have the env var without the trailing slash for the sake of consistency with cmake variables for paths (e.g. the CMAKE_CURRENT_SOURCE_DIR)?

It looks like I am not adding this trailing slash. It gets here all the way from msbuild CoreCLRArtifactsPath

I would rather keep it as is. I mean - rather than detecting and trimming the trailing slash, if present, before exporting in .cmd/.sh

We probably have the same thing for other variables that come from msbuild and just add trailing slashes on concat.
I assume duplicate path separators are ignored on Unix as well as on Windows.

vitek-karas

LGTM

VSadov · 2020-06-26T16:16:42Z

@jkotas - any more comments on this?

jkotas · 2020-06-26T16:22:26Z

Except this, I do not have anything else to add.

Do we have an idea about the impact on local builds and official build times?

VSadov · 2020-06-26T16:39:53Z

@jkotas thanks. I will measure and get actual numbers. (time and size diffs)

janvorli · 2020-06-26T18:12:53Z

On my devbox, the native part of the build (src/coreclr/build-native.cmd) is about 8% slower than the one without this change.

VSadov · 2020-06-27T17:10:55Z

==== Windows build times, shortest from 3 tries

Measuring Release here. Debug build is dominated by crossgen postbuild step and thus much less impacted.
This is a build from a clean state (as in after “git clean -xdf”)

build clr -c Release

master:
Fresh build:
Time Elapsed 00:02:51.03
Rebuild (completely incremental, no changes):
Time Elapsed 00:00:35.39

superhost:
Fresh build:
Time Elapsed 00:03:02.62
Rebuild (completely incremental, no changes):
Time Elapsed 00:00:37.02

==== Linux build times, shortest from 2 tries

I use MUSL containerized build. It generally feels a bit faster and container makes comparing cleaner.
The build is a fresh build after git sync in a fresh container.

./build.sh -s clr -c Release /p:RuntimeOS=Linux-musl

master:
00:04:56.11

superhost:
00:05:07.66

==== File size for coreclr_static library

Windows: 273 Mb
Linux: 398 Mb

Debug is ~20% larger

==== Lab transfers of artifacts (adds roughly 80Mb in either Windows or Linux case)

Baseline (random recent CI run without changes)
Win artefact zip: 99.968MB
Downloaded CoreCLRProduct__Windows_NT_x64_release/CoreCLRProduct__Windows_NT_x64_release.zip to F:\workspace_work\1\s_download_\CoreCLRProduct__Windows_NT_x64_release\CoreCLRProduct__Windows_NT_x64_release.zip
Total Files: 1, Processed: 1, Skipped: 0, Failed: 0, Download time: 7.031 secs, Download size: 99.968MB

Win artifact zip: 181.231MB
Downloaded CoreCLRProduct__Windows_NT_x64_release/CoreCLRProduct__Windows_NT_x64_release.zip to F:\workspace_work\1\s_download_\CoreCLRProduct__Windows_NT_x64_release\CoreCLRProduct__Windows_NT_x64_release.zip
Total Files: 1, Processed: 1, Skipped: 0, Failed: 0, Download time: 12.062 secs, Download size: 181.231MB

=== on Linux the numbers are comparable, but the relative diff is smaller, since baseline is larger due to PAL related artifacts.

Baseline:
Linux artifact zip: 197.283MB
Downloaded CoreCLRProduct__Linux_musl_x64_release/CoreCLRProduct__Linux_musl_x64_release.tar.gz to /datadisks/disk1/workspace/_work/1/s/download/CoreCLRProduct__Linux_musl_x64_release/CoreCLRProduct__Linux_musl_x64_release.tar.gz
Total Files: 1, Processed: 1, Skipped: 0, Failed: 0, Download time: 13.011 secs, Download size: 197.283MB

Linux artifact zip: 278.990MB
Downloaded CoreCLRProduct__Linux_musl_x64_release/CoreCLRProduct__Linux_musl_x64_release.tar.gz to /datadisks/disk1/workspace/_work/1/s/download/CoreCLRProduct__Linux_musl_x64_release/CoreCLRProduct__Linux_musl_x64_release.tar.gz
Total Files: 1, Processed: 1, Skipped: 0, Failed: 0, Download time: 18.014 secs, Download size: 278.990MB

VSadov · 2020-06-27T17:13:19Z

How I see the data above:

=== time:

The change has a fairly small impact on build time.
It adds about 5-6% to fresh build of coreclr Release. The diff is observed only on “warm” repeated builds, otherwise baseline noise could be higher than that.

A lot of effort in this PR went into partitioning coreclr intermediates to minimize duplication while building embedded/standalone flavors of JIT, VM and coreclr. It looks like these efforts paid off.

== size:

The size of coreclr_static static library is 273Mb onWindows, 398 Mb Linux.

That adds ~80 Mb of compressed artifact transfer in the lab, which in turn adds 5 sec of transfer time to the pipeline that consumes the artifact.
For the Installer pipeline the total is ~600 seconds, so the time diff is < 1% (I had only one sample here, this could be noisy, but it is roughly few seconds of diff vs. minutes of total)

== my thoughts:

Overall – I think It would be interesting to try explore whether it is possible to optimize the size of the artifact. This would most likely imply building singlefilehost as a part of coreclr, one way or another. Either via System.Globalization approach or by separating hosting from packaging and moving to coreclr partition.

I think this should be a separate change.

One reason is that it will be an additive improvement, primarily just reducing the intermediate size. Most of the changes of this PR were to minimize duplication and then dealing with consequences of the build partitioning (and platform quirks that were exposed). Most of that will still be needed. Even if singlefilehost is built as a part of coreclr, we do not want to build the same stuff 2-4 times.

Another reason is that, IMO, there is a good chance that the change will be more complex than expected.
The precedent with System.Globalization is a bit optimistic. It is a relatively simple, in terms of dependencies, library consumed by coreclr. I started looking at similar path with System.IO.Compression and it is already not as simple due to dependencies, although seems doable.
Singlefilehost has its own dependencies and then depends on coreclr itself, so good chances that building it from within coreclr build will require a round of build refactorings and then dealing with CMake/linker/OS/arch peculiarities. I am concerned about the time that might take.

I would prefer having experience with embedding native libs before embarking on doing the same with singlefilehost.

VSadov · 2020-06-27T19:33:32Z

logged an issue to follow up on building singlfilehost as a part of coreclr build ( #38489 )

VSadov · 2020-06-29T04:18:02Z

Thanks to everybody!!!

Add PAL_GetApplicationGroupId() that returns null for Linux.

VSadov added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels May 21, 2020

Dotnet-GitSync-Bot added the area-Infrastructure label May 21, 2020

janvorli reviewed May 22, 2020

View reviewed changes

src/coreclr/src/pal/src/loader/module.cpp Outdated Show resolved Hide resolved

jaredpar mentioned this pull request May 25, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

VSadov force-pushed the superhost branch 3 times, most recently from d72a5f2 to 405d3c4 Compare May 27, 2020 23:43

VSadov closed this May 30, 2020

VSadov reopened this May 30, 2020

VSadov force-pushed the superhost branch from 208f604 to 9bfb235 Compare May 30, 2020 15:45

janvorli reviewed Jun 1, 2020

View reviewed changes

src/coreclr/src/debug/ee/wks/CMakeLists.txt Outdated Show resolved Hide resolved

VSadov force-pushed the superhost branch from 0c19313 to b9f9ba1 Compare June 3, 2020 00:14

VSadov force-pushed the superhost branch 12 times, most recently from c04de73 to 2dfb8c9 Compare June 9, 2020 21:48

VSadov force-pushed the superhost branch from bcdccbf to c03f1ad Compare June 10, 2020 13:37

VSadov removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 10, 2020

VSadov added 3 commits June 25, 2020 12:41

since we have files compiled differently between embedded and standal…

c67a9a5

…one coreclr, we do not need EMBEDDED_RUNTIME marker.

a few small fixes

a8cd903

renamed host_resolver_t.h and host_resolver_t.cpp to not have "_t"

fc627ce

VSadov force-pushed the superhost branch from 3ac5426 to fc627ce Compare June 25, 2020 19:41

VSadov requested review from vitek-karas, janvorli and jkotas June 25, 2020 21:28

janvorli approved these changes Jun 26, 2020

View reviewed changes

vitek-karas approved these changes Jun 26, 2020

View reviewed changes

path separating slash when forming CORECLR_STATIC_LIB_LOCATION

1f3217c

VSadov mentioned this pull request Jun 27, 2020

Consider ways to build singlefilehost as a part of coreclr partition build #38489

Closed

VSadov merged commit f93065f into dotnet:master Jun 29, 2020

VSadov deleted the superhost branch June 29, 2020 04:21

swaroop-sridhar mentioned this pull request Jun 30, 2020

Installer build (corehost) for Linux/OSX failing in official build #38590

Closed

mikem8361 mentioned this pull request Jul 1, 2020

Fix SOS/managed debugging failures from PR #36847 #38689

Merged

mikem8361 added a commit to mikem8361/runtime that referenced this pull request Jul 2, 2020

Fix SOS/managed debugging failures from PR dotnet#36847

9d8114f

Add PAL_GetApplicationGroupId() that returns null for Linux.

ghost pushed a commit that referenced this pull request Jul 2, 2020

Fix SOS/managed debugging failures from PR #36847 (#38689)

f170db7

Add PAL_GetApplicationGroupId() that returns null for Linux.

janvorli mentioned this pull request Jul 7, 2020

runtime depends on extremely recent verisons of cmake, blocking adoption on Linux #38755

Closed

janvorli mentioned this pull request Aug 31, 2020

The version string is placed into libcoreclr.so many times #41612

Closed

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Host with coreclr linked in #36847

Host with coreclr linked in #36847

VSadov commented May 21, 2020 •

edited

Loading

ghost commented May 21, 2020

mjsabby commented Jun 3, 2020

VSadov commented Jun 3, 2020

VSadov commented Jun 25, 2020

janvorli Jun 26, 2020

janvorli Jun 26, 2020

VSadov Jun 27, 2020

vitek-karas left a comment

VSadov commented Jun 26, 2020

jkotas commented Jun 26, 2020

VSadov commented Jun 26, 2020

janvorli commented Jun 26, 2020

VSadov commented Jun 27, 2020 •

edited

Loading

VSadov commented Jun 27, 2020 •

edited

Loading

VSadov commented Jun 27, 2020

VSadov commented Jun 29, 2020

Host with coreclr linked in #36847

Host with coreclr linked in #36847

Conversation

VSadov commented May 21, 2020 • edited Loading

ghost commented May 21, 2020

mjsabby commented Jun 3, 2020

VSadov commented Jun 3, 2020

VSadov commented Jun 25, 2020

janvorli Jun 26, 2020

Choose a reason for hiding this comment

janvorli Jun 26, 2020

Choose a reason for hiding this comment

VSadov Jun 27, 2020

Choose a reason for hiding this comment

vitek-karas left a comment

Choose a reason for hiding this comment

VSadov commented Jun 26, 2020

jkotas commented Jun 26, 2020

VSadov commented Jun 26, 2020

janvorli commented Jun 26, 2020

VSadov commented Jun 27, 2020 • edited Loading

VSadov commented Jun 27, 2020 • edited Loading

VSadov commented Jun 27, 2020

VSadov commented Jun 29, 2020

VSadov commented May 21, 2020 •

edited

Loading

VSadov commented Jun 27, 2020 •

edited

Loading

VSadov commented Jun 27, 2020 •

edited

Loading