Bundle size delta

[LesnyRumcajs]

Changes:

• added shellcheck linter to CI, resolved issues with an existing script,
• added bundle-size file denoting the currently expected bundle size. This file gets automatically updated on a push to master (see https://github.com/LesnyRumcajs/builtin-actors/commit/3d4e6f44e8b28ee1c801d0017d6d812e22dd9b1e),
• bot message to every PR summarising WASM bundle size changes - only one message will be posted to the thread, afterwards it will get updated on new commits,
• Makefile rule plus a generator script.

The bundle-size is off by one in this PR to illustrate how it works. It will get updated automatically once it is merged.

There were several ways to accomplish this, but I believe this is the least invasive one for developers while keeping track of the final bundle size and adding it to the PR process.

Closes https://github.com/filecoin-project/builtin-actors/issues/501

[codecov-commenter]

Codecov Report

Merging #937 (c1adeed) into master (5ec2a6b) will decrease coverage by 0.08%. Report is 5 commits behind head on master. The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #937      +/-   ##
==========================================
- Coverage   91.06%   90.98%   -0.08%     
==========================================
  Files         145      145              
  Lines       27798    27798              
==========================================
- Hits        25313    25292      -21     
- Misses       2485     2506      +21     

see 3 files with indirect coverage changes

[github-actions[bot]]

WASM bundle summary 📦

Old bundle size: 6030519
New bundle size: 6030520
Delta:           1 byte(s)

[LesnyRumcajs]

@anorth @jennijuju Given nothing was born out of this one after nine months, should we close this PR or is the change still wanted?

[anorth]

Thanks for raising this, I'm sorry it has languished.

I'm a bit torn here. On the one hand there is some real value here, exploding the bundle size could cause problems. But small changes to it are inevitable and no problem. How likely and quickly are downstream teams or processes to detect a big change in size anyway, even if not automated here? Probably somewhat, but far from perfectly.

On the other hand, simplicity is really important and I'd like to keep our auxilliary code and processes as tight as possible.

On balance I'm inclined to say yet let's merge this. I've reviewed in more detail now and it looks fine. I would just like another actors maintainer to concur before we ship it.

[Stebalien]

This will change for every PR. Can we make it fuzzy and check if we're within 20% of the expected value?

[LesnyRumcajs]

This will change for every PR. Can we make it fuzzy and check if we're within 20% of the expected value?

We could; this would avoid spamming the commit history on master. At the same time, the tracking size value would be decreased. Say I push a commit that increases the bundle size by 15%, and it's accepted. Every consecutive PR would show a message with a large delta until the bundle size file is updated again. I fear folks would ignore those messages. In such a case, maybe the message itself is not worth having, and we could do with, e.g., weekly bundle size updates. If something is really large, it could be investigated manually and potentially reverted. Someone would have to watch it constantly, though.

I'm not sure where lies the right balance to not have spam notifications, relatively fast CI and immediate alerts. What do you think?

[anorth]

I don't see the problem with this changing on every PR. We mandate squashed commits anyway (❤️) so there'll be no noise in the commit history.

Note if we didn't squash commits we could implement this a different way, not with CI magic that makes commits, but just an assertion that the size is right, and implicit requirement that authors need to run the tool to write the correct bundle size before pushing.

[Stebalien]

Hm. As far as I can tell, this change pushes the commit to master so it will cause a commit in master per PR merged. Not a huge issue, but still annoying spam.

However, if we pushed the commit to PR branches, things would get ugly fast (every push would require a pull and rebasing would get annoying).

[anorth]

Hmm, I might have misunderstood the configuration then, and yes I can see how pushing commits to branches would actually be annoying in common workflows of not getting your PR perfect first time. In that case, I agree that this CI-based way of doing it probably won't work for us.

I've used the scheme where CI just checks the value, that must be updated alongside commits to the actual code, in prior projects and it wasn't much pain. Those projects probably had faster CI overall, but it's a thing I'd at least suggest is worth trying here.

[Stebalien]

I don't think that'll work because there is no reproducibility guarantee. On CI, this kind of works because it tends to build in a consistent environment. But I think we need some wiggle room regardless. It can be a percent, but it needs to be there.

[anorth]

Sounds like we need a reproducible build first, then. #171

[LesnyRumcajs]

Yes, this relies on the build being reproducible, which I would assume the CI kind of represents? Of course it's kind of, given that the image for GH runner is not set in stone and may all of a sudden change.

Should we mark it as blocked, then? If so, once there is a reproducible build, how would we want to approach this subject?

Another idea (yet to be tested somewhere) that would not spam the master and limit the amount of annoyance for development is to execute the bundle size update in a merge_group. This would require using merge queues (I highly recommend them, we use them in Forest). The update would be triggered only after the PR is added to the merge queue, which is only possible if all checks are passing and it is approved. The commit would get squashed with others.

[anorth]

Another idea ... is to execute the bundle size update in a merge_group.

We do in fact already use merge queues, though I am also about to suggest we end the experiment using them since I don't think we see the benefits with our work patterns, but it increases latency to land PRs (higher throughput, but we don't have a throughput problem).

However, calculating bundle size there wouldn't give anyone an opportunity to notice changes before committing them. We could bisect later to find big changes though.

[LesnyRumcajs]

However, calculating bundle size there wouldn't give anyone an opportunity to notice changes before committing them. We could bisect later to find big changes though.

We would see the bundle delta as one of the first messages in the PR thread, the same as coverage reports.

[anorth]

However, calculating bundle size there wouldn't give anyone an opportunity to notice changes before committing them. We could bisect later to find big changes though.

We would see the bundle delta as one of the first messages in the PR thread, the same as coverage reports.

As I understand it (and could very well be wrong), using merge_group will only run in the merge queue, i.e. after someone hits "merge" on the PR, so there'd be no commit/comment in the PR. Using pull_request would put a comment where it could be responded to, but then the extra commit causes pain for dev cycles.

The valid paths I see are where CI doesn't make a commit, only checks the value. The value must be set/updated by the dev (via Make) before pushing. For these we either need a reproducible build, or wiggle room in the check. We could prototype it now with the wiggle room, and I think that could work ok.

[LesnyRumcajs]

As I understand it (and could very well be wrong), using merge_group will only run in the merge queue, i.e. after someone hits "merge" on the PR, so there'd be no commit/comment in the PR. Using pull_request would put a comment where it could be responded to, but then the extra commit causes pain for dev cycles.

I believe that with a limited amount of ifs (~2) in the workflow, we could get the good stuff from both worlds:

1. On pull_request event, run the delta against the current value (or better, current main value to avoid potential clashes). Create/update the comment in the PR thread.
2. On merge_group event, update the file.

What do you think?

[anorth]

Let's start with (1). I reckon compare with the current value in file, and allow some small delta to be present without making any PR noise.

• If the author has locally recomputed size (with Make), there'll be no PR noise, but the size change will be obvious from the diff;
• If they haven't, the PR comment will alert them and reviewers to this fact.

We could look to making that second step a merge-blocker in the future, but let's start out informational.

On Tue, 19 Sept 2023 at 19:07, Hubert @.***> wrote:

As I understand it (and could very well be wrong), using merge_group will only run in the merge queue, i.e. after someone hits "merge" on the PR, so there'd be no commit/comment in the PR. Using pull_request would put a comment where it could be responded to, but then the extra commit causes pain for dev cycles.

I believe that with a limited amount of ifs in the workflow, we could get the good stuff from both worlds:

1. On pull_request event, run the delta against the current value (or better, current main value to avoid potential clashes). Create/update the comment in the PR thread.
2. On merge_group event, update the file.

What do you think?

— Reply to this email directly, view it on GitHub https://github.com/filecoin-project/builtin-actors/pull/937#issuecomment-1724945071, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADMW6SJKCMZFD3XGATIWKLX3FAEJANCNFSM6AAAAAAS375CFI . You are receiving this because you were mentioned.Message ID: @.***>

[LesnyRumcajs]

For the record, before I update the current files, in 9 months the bundle size increased by 1.2 MB.