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
Fix Build crashes in releaseFull mode #1980
Merged
WojciechMazur
merged 7 commits into
scala-native:master
from
WojciechMazur:fix/buildReleaseFull
Nov 16, 2020
Merged
Fix Build crashes in releaseFull mode #1980
WojciechMazur
merged 7 commits into
scala-native:master
from
WojciechMazur:fix/buildReleaseFull
Nov 16, 2020
Conversation
This file contains 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
ekrich
reviewed
Nov 2, 2020
This was referenced Nov 3, 2020
sjrd
requested changes
Nov 12, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically looks good. I only suggest to remove the default values.
tools/src/main/scala/scala/scalanative/interflow/MergeProcessor.scala
Outdated
Show resolved
Hide resolved
635f75d
to
d3e5e58
Compare
sjrd
approved these changes
Nov 12, 2020
d3e5e58
to
7e40180
Compare
This was referenced Nov 18, 2020
vicopem
pushed a commit
to vicopem/scala-native
that referenced
this pull request
Nov 19, 2020
* Sub.lub takes optional bound * Pass expected retType to MergeProcessor.toSeq * MergeProcessor mergePhis take additional bound parameter * PolyInline use op.resty as bound for result type * Trait.is should return true when passed j.l.Object * Fix Opt comment * Remove default arguments for dangerous methods
vicopem
added a commit
to vicopem/scala-native
that referenced
this pull request
Nov 19, 2020
This reverts commit a9b8c7d.
vicopem
added a commit
to vicopem/scala-native
that referenced
this pull request
Nov 19, 2020
This reverts commit a9b8c7d.
ekrich
pushed a commit
to ekrich/scala-native
that referenced
this pull request
May 21, 2021
* Sub.lub takes optional bound * Pass expected retType to MergeProcessor.toSeq * MergeProcessor mergePhis take additional bound parameter * PolyInline use op.resty as bound for result type * Trait.is should return true when passed j.l.Object * Fix Opt comment * Remove default arguments for dangerous methods
WojciechMazur
added a commit
to WojciechMazur/scala-native
that referenced
this pull request
Aug 25, 2021
* Sub.lub takes optional bound * Pass expected retType to MergeProcessor.toSeq * MergeProcessor mergePhis take additional bound parameter * PolyInline use op.resty as bound for result type * Trait.is should return true when passed j.l.Object * Fix Opt comment * Remove default arguments for dangerous methods
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #1976
As described in the linked issue SN was crashing in tests when releaseFull mode was used.
The combination of this PR and #1979 allows fixing this problem.
One of the root problems was the wrong calculation of the expected type when using Sub.lub methods. This PR allows passing bound as an optional argument to the mentioned method. Also in most of the usages of this method, we make sure to pass bound type basing on the current context.
To be noted, in one of the recent tests I've discovered that the current Xmx setting of 5G is not sufficient in release full mode - it throws OutOfMemoryError when dumping lowered Defns (the content of all methods is stored in memory, sorted, and then written to file). It's recommended to disable the default nativeDump setting in
MyScalaNativePlugin
or increase Xmx (build passed with Xmx 15G, not tested with the lower value). This problemshould be addressed in later PR.was fixed in #1982Interflow optimization in release full mode took for unit-tests took 51 minutes. Additional ~30 minutes are needed for compilation using clang.