Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm] Emscripten 2.0.34 bump #62499

Merged
merged 28 commits into from
Feb 3, 2022
Merged

Conversation

radekdoulik
Copy link
Member

@radekdoulik radekdoulik commented Dec 7, 2021

Performance wise it looks similar to 2.0.23. Didn't notice any regression here. The managed startup is slightly slower, in second run I measured 223ms, so it is probably just measurement error.

measurement 2.0.23 time 2.0.34
AppStart, Page show 63.0000ms 63.4458ms
AppStart, Reach managed 222.5600ms 226.1304ms
Exceptions, NoExceptionHandling 0.0921us 0.0843us
Exceptions, TryCatch 0.0955us 0.0885us
Exceptions, TryCatchThrow 0.0026ms 0.0026ms
Exceptions, TryCatchFilter 0.1018us 0.0929us
Exceptions, TryCatchFilterInline 0.0869us 0.0800us
Exceptions, TryCatchFilterThrow 0.0033ms 0.0034ms
Exceptions, TryCatchFilterThrowApplies 0.0025ms 0.0025ms
Json, non-ASCII text serialize 9.0376ms 7.9638ms
Json, non-ASCII text deserialize 14.0856ms 11.7460ms
Json, small serialize 0.2351ms 0.2203ms
Json, small deserialize 0.3556ms 0.3359ms
Json, large serialize 69.6667ms 63.6420ms
Json, large deserialize 99.6792ms 92.1754ms
WebSocket, PartialSend 1B 0.0017ms 0.0017ms
WebSocket, PartialSend 64KB 0.0653ms 0.0658ms
WebSocket, PartialSend 1MB 0.9636ms 0.9364ms
WebSocket, PartialReceive 1B 0.0026ms 0.0025ms
WebSocket, PartialReceive 10KB 0.0060ms 0.0040ms
WebSocket, PartialReceive 100KB 0.0000us 0.0000us

@vargaz
Copy link
Contributor

vargaz commented Dec 8, 2021

Might need this as well:

      <_EmccCommonFlags Include="-s ENVIRONMENT=&quot;web,webview,worker,node,shell&quot;" />

Newer versions of emscripten no longer build with 'shell' support by default, i.e. running under v8.

@vargaz
Copy link
Contributor

vargaz commented Dec 9, 2021

Fix for the build failure:
https://gist.github.com/vargaz/4bfce768ed19d283ddc31be124cc82a9

Radek Doulik and others added 4 commits December 9, 2021 17:02
Co-authored-by: Zoltan Varga <vargaz@gmail.com>
    src/mono/mono/mini/mini-runtime.c:3407:25: error: ‘invoke’ undeclared (first use in this function); did you mean ‘revoke’?
       3407 |                         invoke = mono_marshal_get_runtime_invoke_dynamic ();
Environment setting https://github.com/emscripten-core/emscripten/blob/2.0.34/src/settings.js#L616-L641

From emscripten 2.0.25 release notes

    - Support for the 'shell' environment is now disabled by default.  Running under
      `d8`, `js`, or `jsc` is not something that most emscripten users ever want to
      do, so including the support code is, more often than not, unnecessary.  Users
      who want shell support can enable it by including 'shell' in `-s ENVIRONMENT`
      (dotnet#14535).

Example of the the size increase for bench sample:

    -a---          12/10/2021  3:35 PM         382113 dotnet.js
    -a---          12/13/2021 10:37 AM         383589 dotnet.js
@radekdoulik
Copy link
Member Author

Related: #62708

src/mono/wasm/wasm.proj Outdated Show resolved Hide resolved
@lewing
Copy link
Member

lewing commented Jan 31, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lewing
Copy link
Member

lewing commented Feb 1, 2022

looks like we need to bump Emscripten in icu dotnet/icu#177

@lewing
Copy link
Member

lewing commented Feb 1, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lewing lewing marked this pull request as ready for review February 1, 2022 15:48
@radekdoulik
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

To avoid these errors:

WasmApp.Native.targets(342,5): error : Failed to compile .../Microsoft.CodeAnalysis.CSharp.dll.bc -> /datadisks/disk1/work/B9F209B7/w/B1710A2F/e/wasm_build/obj/wasm/for-build/Microsoft.CodeAnalysis.CSharp.dll.o
WasmApp.Native.targets(342,5): error : emcc: warning: linker setting ignored during compilation: 'TOTAL_MEMORY' [-Wunused-command-line-argument]
WasmApp.Native.targets(342,5): error : emcc: warning: linker setting ignored during compilation: 'ERROR_ON_UNDEFINED_SYMBOLS' [-Wunused-command-line-argument]
@radekdoulik
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radekdoulik
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radekdoulik
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

lewing and others added 2 commits February 2, 2022 17:31
When running with nodejs, the managed app would exit with code 42, but
node would exit with 1. Use `process.exit` for node, instead of
`mono_wasm_exit`.
@radical
Copy link
Member

radical commented Feb 3, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented Feb 3, 2022

Failures:

These will be investigated after this PR is merged.

@radical radical merged commit 724c4e1 into dotnet:main Feb 3, 2022
@sbomer
Copy link
Member

sbomer commented Feb 3, 2022

Why was this merged with known runtime-extra-platforms failures? I ask because we are trying to keep get runtime and runtime-extra-platforms to green to make it easier to detect new failures. Would it be reasonable to disable the failing tests before merging in the future? Just curious what we can do to meet our build health goals without unnecessarily slowing down development.

@radical
Copy link
Member

radical commented Feb 3, 2022

Why was this merged with known runtime-extra-platforms failures?

This was part of a bigger mess, which started with an update dependencies PR from emsdk getting merged, which bumped the emscripten version being used, which broken Wasm.Build.Tests (on main, and PRs), which got mixed with many helix queues getting removed, and all that combining to cause a badly red main, and lots of PRs being blocked. Merging this unblocked them, and made main much less red. And I opened a PR to disable those tests today (#64759).

I ask because we are trying to keep get runtime and runtime-extra-platforms to green to make it easier to detect new failures. Would it be reasonable to disable the failing tests before merging in the future? Just curious what we can do to meet our build health goals without unnecessarily slowing down development.

I completely agree with that, and avoid merging with any failures caused by the PR. But this was an unusual case.

@sbomer
Copy link
Member

sbomer commented Feb 3, 2022

Thanks @radical, I appreciate the explanation

radekdoulik added a commit to radekdoulik/runtime that referenced this pull request Feb 17, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants