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

Compare binary size with latest master build on CI #91715

Closed
wants to merge 1 commit into from

Conversation

fire
Copy link
Member

@fire fire commented May 8, 2024

This makes it easier to track binary size regressions, as you no longer have to build the editor and/or export templates locally to do so.

@Calinou Can you link the proposal number for your pr?

  • Copy the correct numbers for the hard coded values

@fire fire requested a review from a team as a code owner May 8, 2024 14:49
@fire fire force-pushed the ci-compare-binary-size branch 7 times, most recently from a67ef5d to c42d3af Compare May 8, 2024 15:15
@Calinou Calinou added this to the 4.x milestone May 8, 2024
@Calinou
Copy link
Member

Calinou commented May 8, 2024

I don't think comparing the binary size with hardcoded sizes is the right approach. I want to see on CI if the PR's build is larger than the latest successful master build on CI, disregarding official releases. We can't use official releases as a basis here, as they're built with different SCons options that lead to much smaller binaries. (They're also built on a different toolchain, which will also result in different binary sizes.)

@Calinou Can you link the proposal number for your pr?

I've opened a proposal to detail the intent of this feature: godotengine/godot-proposals#9705

@fire
Copy link
Member Author

fire commented May 8, 2024

I disagree, we can use the data from for example github actions' 4.2 branch. The idea is that the hardcoded values don't change every commit.

@Calinou
Copy link
Member

Calinou commented May 8, 2024

I disagree, we can use the data from for example github actions' 4.2 branch. The idea is that the hardcoded values don't change every commit.

If we do this, then we don't know anymore if a PR is actually helping reduce Godot's binary size, or is increasing it.

For example:

  • 4.2 branch's latest build is 33,000,000 bytes.
  • master branch's latest build is 34,000,000 bytes.
  • PR X decreases binary size by 40,000 bytes over master. Compared to 4.2, PR X appears 960,000 bytes larger.
  • PR Y increases binary size by 1,500 bytes over master. Compared to 4.2, PR Y appears 998,500 bytes larger.

Both PRs appear to result in a huge binary size increase, but one is actually doing a great job at reducing binary size compared to the status quo (master), while the other isn't.

@fire
Copy link
Member Author

fire commented May 8, 2024

Maybe we can store the last build size on cache?

@Calinou
Copy link
Member

Calinou commented May 8, 2024

Maybe we can store the last build size on cache?

We could have a dedicated artifact for this if we really want to avoid downloading previous builds, but I don't think that's a big issue. Make sure to run strip on the binaries beforehand before uploading their size somewhere though.

@fire fire force-pushed the ci-compare-binary-size branch 2 times, most recently from 1863c7c to 137b12d Compare May 8, 2024 16:25
@fire fire force-pushed the ci-compare-binary-size branch 7 times, most recently from 57760f1 to fa364e8 Compare May 8, 2024 17:12
@fire fire force-pushed the ci-compare-binary-size branch 6 times, most recently from 070c8ee to 2e2e6c3 Compare May 8, 2024 19:53
This makes it easier to track binary size regressions, as you no longer
have to build the editor and/or export templates locally to do so.

Try to combine the workspace.
@fire fire marked this pull request as draft May 8, 2024 20:14
@fire
Copy link
Member Author

fire commented May 30, 2024

This is stuck looking for a way to determine "last" logical commit and downloadd, executing it to calculate binary size.

@fire fire closed this May 30, 2024
@akien-mga akien-mga removed this from the 4.x milestone May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants