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

perf: remove lto setting from CLI #6861

Merged
merged 5 commits into from
Aug 14, 2023

Conversation

CrabNejonas
Copy link
Contributor

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

This PR removes the lto=true setting for the CLI. This drastically improves the install performance (the time it takes to run cargo install tauri-cli).
Since the CLI is not doing performance-critical work and size is not important here since the cli will be compiled locally this does not have a downside it it afaik and only improves performance for rust users.

To give you an idea here are the install times on my MacBook Pro m2:

build time
with lto 2:55.31
without lto 1:05.46

so as you can see disabling lto reduced the compile-time by 3x.

@CrabNejonas CrabNejonas requested a review from a team as a code owner May 4, 2023 14:21
@FabianLars
Copy link
Member

what do you think about moving the lto (and the new stuff you added in the js api) into a custom profile that inherits the release profile so that we can use that for the cargo-binstall CI?

@lucasfernog
Copy link
Member

I had to use patch-package because napi-rs does not support custom profiles :(

@lucasfernog
Copy link
Member

Also @JonasKruckenberg why did you target next here instead of dev?

@amrbashir
Copy link
Member

I had to use patch-package because napi-rs does not support custom profiles :(

I would probably look into upstreaming this to napi-rs, the maintainer is pretty responsive and would probably publish it soon after merging.

Also if I understand correctly, this change wants to improve cargo install tauri-cli which iirc will use release profile by default which doesn't have this change and users would need to cargo install tauri-cli --profile release-size-optimized which is bad DX

@lucasfernog
Copy link
Member

He's focusing on v3 so I'm not sure if he wants to patch for v2. I'll try anyway.

@lucasfernog
Copy link
Member

I had to use patch-package because napi-rs does not support custom profiles :(

I would probably look into upstreaming this to napi-rs, the maintainer is pretty responsive and would probably publish it soon after merging.

Also if I understand correctly, this change wants to improve cargo install tauri-cli which iirc will use release profile by default which doesn't have this change and users would need to cargo install tauri-cli --profile release-size-optimized which is bad DX

I think you misunderstood. We don't want users to use release-size-optimized, we want them to have faster builds. The size optimization will be used only on the cargo-binstall and NPM assets.

@amrbashir
Copy link
Member

ah right, I was even more confused by the name. LGTM then.

amrbashir
amrbashir previously approved these changes May 26, 2023
FabianLars
FabianLars previously approved these changes May 26, 2023
@lucasfernog
Copy link
Member

@CrabNejonas we also need to rebase this branch with commit signing 😢

@lucasfernog lucasfernog dismissed stale reviews from FabianLars and amrbashir via c8cdba0 May 27, 2023 13:52
@socket-security
Copy link

socket-security bot commented May 27, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@lucasfernog lucasfernog changed the base branch from next to dev June 16, 2023 16:00
@lucasfernog lucasfernog merged commit 02eb08b into tauri-apps:dev Aug 14, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 In audit
Development

Successfully merging this pull request may close these issues.

None yet

4 participants