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.

Sign up for GitHub

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

Jump to bottom

feat: Track the binary size for all the different platforms #34744

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Track the binary size for all the different platforms #34744

wants to merge 1 commit into from
+280 −76

Conversation

dklassic
Copy link
Contributor

@dklassic dklassic commented Dec 23, 2024 •
edited
Loading

For now what it does are:

  • Add bencher step for each individual platforms, will run on full workflow
  • Currently we only track build size in platforms other than Linux
  • Build Size are tracked in:
    • macos: macos-${{ inputs.profile }}
    • Windows: windows-${{ inputs.profile }}
    • Linux: linux-${{ inputs.profile }}
    • Android: android-${{ matrix.target }}-${{ inputs.profile }}
    • Open Harmony: ohos-${{ matrix.target }}-${{ inputs.profile }}
  • macOS, Windows, and Linux will be additional stripped to test the stripped binary size

The linux-bencher.yml has been heavily modified to be a generic bencher workflow for any target.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix part of Servo on Bencher improvements #34555 (intentional wording to prevent automatically closing the issue)
  • There are tests for these changes in try parser tests

@dklassic
Copy link
Contributor Author

dklassic commented Dec 23, 2024 •
edited
Loading

Some unsure points:

  • Not sure if the target-profile naming is good enough, especially for Android and Open Harmony the targets are all named with platform at the end (e.g. x86_64-linux-android) which is sorta hard to read. Switched to a {platform}-{target} scheme.
  • For now it only tracks release, supposedly we care more about production and production-stripped then I guess we'll have to basically build additional production binary at least for every platform for each main PR land, which may somewhat bloat the duration for our workflow.
  • Another way is to track only nightly releases which can be done on internal-wpt-dashboard runs as suggested by @sagudev or maybe pass an argument to allow size tracking workflow to run during nightly builds, which is maybe more of a no-brainer.

Edit: Currently all resolved.

I've tested a full run with additional production profile and the duration is actually the same as a release profile only, because ultimately the bottleneck is WPT tests and production profile actually runs pretty fast.

I think we can just do everything all at once for each commit. Here's a full run: https://github.com/dklassic/servo/actions/runs/12485231562

@dklassic dklassic force-pushed the build-size-tracking branch from 6922cac to 3d9cbac Compare December 23, 2024 04:02
@dklassic
Copy link
Contributor Author

dklassic commented Dec 23, 2024 •
edited
Loading

image

Also I notice that bencher seems to report in MB now but the unit remains to be Bytes

I've reported an upstream issue for this: bencherdev/bencher#545

@sagudev
Copy link
Member

sagudev commented Dec 23, 2024

Wow, that's a lot of files. Can we remove repetition by making bencher workflow more generic (argument to accept os to get full binary name, all test to do in bencher bin, speedometer, dromeo) or at least use composable github action.

@dklassic
Copy link
Contributor Author

dklassic commented Dec 23, 2024 •
edited
Loading

Wow, that's a lot of files. Can we remove repetition by making bencher workflow more generic (argument to accept os to get full binary name, all test to do in bencher bin, speedometer, dromeo) or at least use composable github action.

Yeah that's probably the way to go, I'm just naively hacking it out to see if the overall workflow makes sense or not.

@dklassic dklassic force-pushed the build-size-tracking branch from cc1f3fd to 26c6fef Compare December 23, 2024 07:23
@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

4 similar comments
@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@dklassic dklassic force-pushed the build-size-tracking branch from 684ddc5 to b35ddd8 Compare December 24, 2024 13:03
@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@dklassic dklassic force-pushed the build-size-tracking branch from b35ddd8 to 922c253 Compare December 24, 2024 13:47
@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

1 similar comment
@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@dklassic dklassic marked this pull request as ready for review December 25, 2024 02:38
@dklassic dklassic requested a review from jschwe as a code owner December 25, 2024 02:38
@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@dklassic
Copy link
Contributor Author

dklassic commented Dec 25, 2024 •
edited
Loading

So I've tested a full run with additional production profile and the duration is actually the same as a release profile only, because ultimately the bottleneck is WPT tests and production profile actually runs pretty fast.

I think we can just do everything all at once for each commit. Here's a full run: https://github.com/dklassic/servo/actions/runs/12485231562

@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

2 similar comments
@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@servo-wpt-sync
Copy link
Collaborator

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

@mrobinson
Copy link
Member

@dklassic Please squash and rebase before merging. Each commit is a line in the final commit message, so 60 commits will be quite long.

@dklassic dklassic force-pushed the build-size-tracking branch 3 times, most recently from a9e978a to 3b96fc0 Compare December 26, 2024 00:38
@dklassic
Copy link
Contributor Author

dklassic commented Jan 3, 2025 •
edited
Loading

@jschwe just pinging to make sure this PR is not lost in all the moving parts. Feels like we should get it running soon and start tracking the numbers if there’s no other complications.

wusyong
wusyong approved these changes Jan 6, 2025
View reviewed changes
Copy link
Member

@wusyong wusyong left a comment

Choose a reason for hiding this comment

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

LGTM and I think we should start tracking the size if possible.
If no one disagrees, I'll merge this later.

sagudev
sagudev reviewed Jan 6, 2025
View reviewed changes
python/servo/try_parser.py Outdated Show resolved Hide resolved
@dklassic dklassic force-pushed the build-size-tracking branch from bba9846 to 2e19ac0 Compare January 6, 2025 17:23
sagudev
sagudev reviewed Jan 6, 2025
View reviewed changes
python/servo/try_parser.py Outdated Show resolved Hide resolved
@sagudev sagudev requested a review from mrobinson January 6, 2025 20:34
mrobinson
mrobinson requested changes Jan 7, 2025
View reviewed changes
python/servo/try_parser.py Outdated Show resolved Hide resolved
@dklassic dklassic force-pushed the build-size-tracking branch 2 times, most recently from 5a65ee4 to cef1fd7 Compare January 8, 2025 04:05
@dklassic
Copy link
Contributor Author

dklassic commented Jan 8, 2025 •
edited
Loading

image

https://github.com/dklassic/servo/actions/runs/12665377293

Added workflow setup to be shown on the job name, should look way clearer what each workflow is doing now.

@dklassic dklassic requested review from mrobinson and sagudev January 8, 2025 07:39
sagudev
sagudev reviewed Jan 8, 2025
View reviewed changes
python/servo/try_parser.py Outdated Show resolved Hide resolved
@dklassic dklassic force-pushed the build-size-tracking branch 2 times, most recently from 89d2cfb to e16e658 Compare January 8, 2025 11:18
dklassic
dklassic commented Jan 8, 2025
View reviewed changes
python/servo/try_parser.py Outdated Show resolved Hide resolved
@dklassic
Copy link
Contributor Author

dklassic commented Jan 8, 2025

https://github.com/dklassic/servo/actions/runs/12675323101

Okay I think I got it all ready.

@dklassic
feat: Track the binary size for all the different platforms
8f38b97 
Signed-off-by: DK Liao <dklassic@gmail.com>
@dklassic dklassic force-pushed the build-size-tracking branch from 383b7a6 to 8f38b97 Compare January 8, 2025 18:43
@dklassic
Copy link
Contributor Author

@sagudev do you mind see if there's any other improvements that can be made?

jschwe
jschwe reviewed Jan 10, 2025
View reviewed changes
.github/workflows/bencher.yml
echo "SERVO_FILE_SIZE_RESULT=size.json" >> "$GITHUB_ENV"
# We'll additionally strip and measure the size of the binary when using production profile
- name: Install LLVM
if: ${{ inputs.stripped-file-size == true }}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this additional input option? Couldn't just check if profile == production?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For ohos binaries, stripping them with llvm directly doesn’t seem to work atm.

Copy link
Member

Choose a reason for hiding this comment

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

What kind of errors are you encountering? I've been using the standard llvm installed on my system to strip libservoshell.so without problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose I'm doing it wrongly then, here's the error:

https://github.com/dklassic/servo/actions/runs/12481534299/job/34834716773#step:13:21

Copy link
Member

Choose a reason for hiding this comment

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

Ah, We shouldn't be measuring the .hap file, we should measure the libservoshell.so dynamic library, since that is what we develop here. The .hap is kind of like an apk file on android. For android I would recommend the same, measure libservoshell.so, since that is where our code is.

Copy link
Contributor Author

@dklassic dklassic Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah right, that would make sense, I'll make changes on that.

While we're at it, may I borrow your wisdom on Android Production profile? I'm not sure if it is expected but simply running Android workflow with Production profile will lead to the following error:

https://github.com/dklassic/servo/actions/runs/12488632199/job/34851264188#step:14:1108

So currently the setup skips production build on Android, but maybe it's just me missing something again.

Copy link
Member

@jschwe jschwe Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, yes I encountered that issue when building android too. It comes from here:

servo/python/servo/package_commands.py

Line 164 in 73c0701

raise Exception("TODO what should this be?")
and would require to go over the mach file and check in which cases the profile is meant for the android APK build (which should be either release or debug) vs for the rust servoshell library (which can be any arbitrary custom profile). So we would probably need to map it.

Edit: I see I had opened an issue: #34564

jschwe
jschwe reviewed Jan 10, 2025
View reviewed changes
.github/workflows/linux.yml
stripped-file-size: ${{ inputs.profile == 'production' }}
# We only evaluate speedometer and dromaeo score in release
speedometer: ${{ inputs.profile == 'release' }}
dromaeo: ${{ inputs.profile == 'release' }}
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't speedometer and dromaeo be release or production?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can track the scores for production profile as well, not sure if warranted atm though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jschwe jschwe jschwe approved these changes

@sagudev sagudev sagudev left review comments

@wusyong wusyong wusyong approved these changes

@mrobinson mrobinson Awaiting requested review from mrobinson

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dklassic @sagudev @servo-wpt-sync @mrobinson @wusyong @jschwe