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

Remove all unsafe linker flags from Package.swift #91

Merged
merged 3 commits into from
Oct 3, 2020

Conversation

kateinoigakukun
Copy link
Member

@kateinoigakukun kateinoigakukun commented Oct 2, 2020

Need toolchain including swiftwasm/llvm-project@20c933e

Resolve #6

@kateinoigakukun kateinoigakukun force-pushed the omit-unsafe-linker-flags branch from 36b2e89 to dbfb722 Compare October 2, 2020 14:55
@kateinoigakukun kateinoigakukun force-pushed the omit-unsafe-linker-flags branch 3 times, most recently from 2094101 to 61a6b65 Compare October 3, 2020 02:04
@github-actions
Copy link

github-actions bot commented Oct 3, 2020

Time Change: -3,591.5ms (38%) 🎉

Total Time: 9,306.5ms

Test name Duration Change
Serialization/Swift Int to JavaScript 3,044ms -1,677.5ms (55%) 🏆
Serialization/Swift String to JavaScript 3,118.5ms -1,638.25ms (52%) 🏆
Object heap/Increment and decrement RC 2,787.75ms -285.25ms (10%) 👏
ℹ️ View Unchanged
Test name Duration Change
Serialization/Write JavaScript number directly 176.25ms +2.5ms (1%)
Serialization/Write JavaScript string directly 180ms +7ms (3%)

performance-action

@@ -16,6 +16,7 @@ jobs:
export SWIFTENV_ROOT="$HOME/.swiftenv"
export PATH="$SWIFTENV_ROOT/bin:$PATH"
eval "$(swiftenv init -)"
./scripts/install-toolchain.sh
swiftenv install $TOOLCHAIN_DOWNLOAD
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to remove swiftenv install line after merging this PR because older version toolchain is necessary to compare with base branch

@kateinoigakukun kateinoigakukun force-pushed the omit-unsafe-linker-flags branch from 61a6b65 to bb3a3e0 Compare October 3, 2020 02:11
@kateinoigakukun
Copy link
Member Author

The reason for the performance improvement is that we started to build toolchain without assertion.

@j-f1
Copy link
Member

j-f1 commented Oct 3, 2020

Is that expected? I’m pretty sure the benchmarks only measure runtime performance, not built time. Or am I missing something about what “building without assertions” means?

@kateinoigakukun
Copy link
Member Author

@j-f1 It's an expected result. Because stdlib binary is built without any assert(condition), the runtime performance is improved.

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Oct 3, 2020

Do you know if upstream distributions disable assertions by the way? I thought that we disabled assertions temporarily until swiftwasm/swift#1823 is fixed.

@kateinoigakukun
Copy link
Member Author

@MaxDesiatov IMO, we should disable assertion for distribution toolchains even swiftwasm/swift#1823 will be fixed. I'm not sure which build-preset is used for upstream toolchain distribution, but they use no-assertion for release version toolchain in general.

@kateinoigakukun
Copy link
Member Author

@MaxDesiatov @j-f1 Can we merge this PR or do I need more detail description?

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, I look forward to building Tokamak on Linux or macOS 🙂

@kateinoigakukun kateinoigakukun merged commit 4b7981b into main Oct 3, 2020
@kateinoigakukun kateinoigakukun deleted the omit-unsafe-linker-flags branch October 3, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build fails with the unsafe flags error
3 participants