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: Add conservative garbage collection #7992

Merged
merged 2 commits into from Mar 24, 2020
Merged

Conversation

@yowl
Copy link
Contributor

yowl commented Feb 18, 2020

Enables conservative garbage collection in the Wasm backend. WIP as would like to solicit suggestions for tests. Naively, I could loop and create some large arrays beyond the memory limit, which should cause a GC and succeed, but anything better/more sophisticated I should add to the tests? So far the only test is to create and collect an object, testing through a WeakReference.

WIP also as depends on #7983 at which point 99% of the diffs should disappear.

Closes #5305

@yowl

This comment has been minimized.

Copy link
Contributor Author

yowl commented Feb 18, 2020

One of the errors here is

  /usr/bin/clang
  Generating native code
  The method or operation is not implemented. ([S.P.CoreLib]System.Globalization.CultureNotFoundException..ctor(SerializationInfo,StreamingContext))
  The method or operation is not implemented. ([S.P.CompilerGenerated]Internal.CompilerGenerated.<Module>.InvokeRetO<Nullable`1<int32>>(object,native int,ArgSetupState&,bool))
  The method or operation is not implemented. ([S.P.CoreLib]System.Globalization.TimeSpanParse+TimeSpanResult.SetBadFormatSpecifierFailure(Nullable`1<char>))
Running test /Users/runner/runners/2.164.8/work/1/s/tests/src/Simple/Hello Hello
fail

@MichalStrehovsky

This comment has been minimized.

Copy link
Member

MichalStrehovsky commented Feb 19, 2020

I've merged the LLVM upgrade - could you rebase on top of master to make the diff more reviewable?

(I'm asking for rebase, but I would probably just do the low-tech thing and copy all the files to the side, git checkout master, delete the branch, create branch with the exact same name, and copy the files back, commit, and force push to origin. I'm not a git wizard myself and rebasing so many commits always leaves me re-resolving the same merge conflicts)

@yowl yowl force-pushed the yowl:gc branch from e7acaa8 to f9423b7 Feb 19, 2020
@yowl

This comment has been minimized.

Copy link
Contributor Author

yowl commented Feb 19, 2020

Reset to reduce the commit mess. I did a reset in the end, https://stackoverflow.com/questions/46733179/git-extensions-squash-commits

@Suchiman

This comment has been minimized.

Copy link
Contributor

Suchiman commented Feb 19, 2020

Should this remove the artificial high gen0 budget for wasm here?

#ifdef USE_PORTABLE_HELPERS
// CORERT-TODO: remove this
// https://github.com/dotnet/corert/issues/2033
*value = 100 * 1024 * 1024;
#else
*value = 0;
#endif

Problem is that it's still needed for CPPCodeGen AFAIK

@MichalStrehovsky

This comment has been minimized.

Copy link
Member

MichalStrehovsky commented Feb 20, 2020

Naively, I could loop and create some large arrays beyond the memory limit, which should cause a GC and succeed, but anything better/more sophisticated I should add to the tests?

Yes, a test like that would be good. We should be able to run some dedicated tests for the GC eventually. Unfortunately a lot of CoreCLR GC tests rely on precise stack scanning - we could either try to find Mono's GC tests (I couldn't find them when I quickly looked around though), or wait until Mono team is done enabling running CoreCLR's tests with Mono. They'll have to do the legwork of figuring out which tests are expected to work with a conservative GC.

Should this remove the artificial high gen0 budget for wasm here?

Yup, we could. We can turn the ifdef into defined(USE_PORTABLE_HELPERS) && !defined(_WASM_).

comment out assert for m_pHackPInvokeTunnel - how does this fit in with Wasm and a shadow stack
@yowl

This comment has been minimized.

Copy link
Contributor Author

yowl commented Mar 24, 2020

From what I can see m_pHackPInvokeTunnel is set in assembler so I dont know if it's relevant for Wasm? Going to remove the WIP to welcome feedback.

@yowl yowl changed the title WIP: Wasm: Add conservative garbage collection Wasm: Add conservative garbage collection Mar 24, 2020
@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Mar 24, 2020

m_pHackPInvokeTunnel

This should not be needed for WASM. It is needed for regular CPUs to get calllee saved registers that are not a thing for WASM.

@jkotas
jkotas approved these changes Mar 24, 2020
Copy link
Member

jkotas left a comment

LGTM. @MichalStrehovsky Do you have any feedback?

Copy link
Member

MichalStrehovsky left a comment

Looks good! Thank you!

@MichalStrehovsky MichalStrehovsky merged commit 592a2ca into dotnet:master Mar 24, 2020
12 checks passed
12 checks passed
WIP Ready for review
Details
corert-ci Build #20200221.8 succeeded
Details
corert-ci (Build Linux x64 debug and CoreCLR tests) Build Linux x64 debug and CoreCLR tests succeeded
Details
corert-ci (Build Linux x64 debug and CoreFX tests) Build Linux x64 debug and CoreFX tests succeeded
Details
corert-ci (Build Linux x64 release) Build Linux x64 release succeeded
Details
corert-ci (Build OSX x64 debug and CoreCLR tests) Build OSX x64 debug and CoreCLR tests succeeded
Details
corert-ci (Build OSX x64 debug and CoreFX tests) Build OSX x64 debug and CoreFX tests succeeded
Details
corert-ci (Build OSX x64 release) Build OSX x64 release succeeded
Details
corert-ci (Build Windows_NT x64 debug and CoreCLR tests) Build Windows_NT x64 debug and CoreCLR tests succeeded
Details
corert-ci (Build Windows_NT x64 debug and CoreFX tests) Build Windows_NT x64 debug and CoreFX tests succeeded
Details
corert-ci (Build Windows_NT x64 release) Build Windows_NT x64 release succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.