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

fix: target-cpu=native flag working again #4498

Merged
merged 1 commit into from Mar 20, 2024

Conversation

jbencin
Copy link
Contributor

@jbencin jbencin commented Mar 6, 2024

Description

Pass the flag target-cpu=native to rustc for local builds and tests for improved performance

This PR also require stacks-network/actions#31 to be approved and merged

Applicable issues

@jbencin jbencin requested review from wileyj and obycode March 6, 2024 22:00
@wileyj
Copy link
Contributor

wileyj commented Mar 6, 2024

this changes quite a lot when considering ti's for

Pass the flag target-cpu=native to rustc for local builds and tests for improved performance

what it is goal of this PR?

@jbencin
Copy link
Contributor Author

jbencin commented Mar 6, 2024

this changes quite a lot when considering ti's for

It touches 12 of files, but it's only adding a single word in 11 of them

what it is goal of this PR?

It's a few percent perf improvements on local builds. See the discussion on the Slack #performance channel. I added this in #4363, but it was broken in #4489

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.21%. Comparing base (0e9cf5d) to head (8ff05eb).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4498      +/-   ##
==========================================
- Coverage   83.32%   83.21%   -0.12%     
==========================================
  Files         453      453              
  Lines      328494   328494              
  Branches      323      323              
==========================================
- Hits       273731   273342     -389     
- Misses      54755    55144     +389     
  Partials        8        8              

see 25 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e9cf5d...8ff05eb. Read the comment docs.

@wileyj
Copy link
Contributor

wileyj commented Mar 6, 2024

this changes quite a lot when considering ti's for

It touches 12 of files, but it's only adding a single word in 11 of them

what it is goal of this PR?

It's a few percent perf improvements on local builds. See the discussion on the Slack #performance channel. I added this in #4363, but it was broken in #4489

i guess that's my question - you've mentioned "local builds" twice now, but there are only 2 Dockerfiles that should ever be used to build locally: Dockerfile and Dockerfle.debian. the rest are quite specific to PR and release builds.
you could make the case to add the change here as well: ./.github/actions/dockerfiles/Dockerfile.debian-source.

@wileyj
Copy link
Contributor

wileyj commented Mar 6, 2024

if #4489 added a regression, wouldn't it be simpler to revert that change vs adding more changes?

Copy link
Contributor

@wileyj wileyj left a comment

Choose a reason for hiding this comment

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

requesting changes as a placeholder, i think a call is in order

@wileyj
Copy link
Contributor

wileyj commented Mar 7, 2024

@jbencin can PR the ./build-scripts changes here as well? there is an open PR to start using these files that hasn't merged into next yet.
https://github.com/stacks-network/actions/tree/main/stacks-core/create-source-binary/build-scripts

we can discuss concerns around removing these from the stacks-core repo elsewhere, my main concern is merging this PR, then not having these chagnes in teh stacks-core/build-scripts is removed in another open PR

wileyj
wileyj previously approved these changes Mar 7, 2024
Copy link
Contributor

@wileyj wileyj left a comment

Choose a reason for hiding this comment

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

shipit

@wileyj
Copy link
Contributor

wileyj commented Mar 7, 2024

ref: #4497

obycode
obycode previously approved these changes Mar 12, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

ok, I think this seems fine. 👍

@jbencin jbencin dismissed stale reviews from obycode and wileyj via 14e5b79 March 12, 2024 20:43
@obycode
Copy link
Contributor

obycode commented Mar 13, 2024

Looks like there are still conflicts here @jbencin

@wileyj
Copy link
Contributor

wileyj commented Mar 13, 2024

Looks like there are still conflicts here @jbencin

it's related to #4497

jcnelson
jcnelson previously approved these changes Mar 15, 2024
Copy link
Contributor

@wileyj wileyj left a comment

Choose a reason for hiding this comment

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

shipit

@wileyj wileyj enabled auto-merge March 20, 2024 17:23
@wileyj wileyj added this pull request to the merge queue Mar 20, 2024
Merged via the queue into stacks-network:next with commit 1c5b5d1 Mar 20, 2024
2 checks passed
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.

None yet

4 participants