Page MenuHomePhabricator

Bundlesize test should be updated to warn rather than fail when insignificant bytes increase
Open, LowPublic

Description

Bundlesize is a useful for checking if a change results in a significant filesize increase, but setting the hard limits to the exact current filesize is often very frustrating for developers and not that useful. Using the tool like this will cause a V-1 even when the size increases by a few bytes, but will also miss more significant increases if the filesize dropped recently without the limit being decreased, e.g.

False positive:
FAIL skins.vector.styles: 9.31KB > maxSize 9.3KB (gzip)
False negative:
PASS skins.vector.search: 74B < maxSize 2.8KB (gzip)

(from https://gerrit.wikimedia.org/r/c/mediawiki/core/+/651848 )

The tool should be configured like a code coverage tool: reports should show % increases and warn if these are above a certain limit. If a fixed limit is desired, that should be a value that we would consider to be too high for some time to come, so it isn't expected to be continuously updated.

Event Timeline

It could even fail, and give V-1, when the change is so insignificant to the extent that we end up with strange error due to rounding, see T268593: skins.vector.styles.legacy: 8KB > maxSize 8KB (gzip) . I don't think it should even fail in such cases.

Legoktm subscribed.

This is not an issue in the Wikimedia CI system, but a configuration that is in Vector (legacy skin) and under the control of that component's maintainers. See https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/skins/Vector/+/refs/heads/master/bundlesize.config.json

This is for discussing guidance that would apply to multiple extensions/skins. I don’t know if there is a more appropriate tag, but it is not just Vector.

I don't think we should be rounding up to 0.1kb. That's silly IMO and I suspect rounding up by say 0.5kb would reduce the frequency of these errors.

Tbh I don't think warnings would be very helpful for this. If an additional 1kb is added to a bundle (which has happened) that's a big dent in performance and worth the annoyance of a bundlesize error IMO.

The error should only happen in core in the rare case the change influences one of the ResourceLoaderSkinModules used by Vector and with the recent changes to the site notice we saw this which personally was a useful part of the review process. I think it is also useful as part of the message box change referenced in the description to make sure we consider the impacts on bundle size with the change. These changes impact all skins and we should consider the performance impact.

I don't think we should be rounding up to 0.1kb. That's silly IMO and I suspect rounding up by say 0.5kb would reduce the frequency of these errors.

It would reduce the frequency, but only in a pseudo-random way. Any bundles of size X.49 or X.99 would be very sensitive, whereas bundles of size X.01 or X.51 would not.

Tbh I don't think warnings would be very helpful for this.

This is what non-voting CI warnings should be used for. I think it's a matter of process to make sure they are checked as part of the review process, and making sure they are visible enough.

If an additional 1kb is added to a bundle (which has happened) that's a big dent in performance and worth the annoyance of a bundlesize error IMO.

Agreed, but the current system requires developers to always remember to adjust the bundle sizes down when the file sizes shrink, for which there is currently no reminder to do so, so you end up with the false negatives described in the task. By using a CI warning you will always be comparing the new size with the exact size before the patch.

Change 663615 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Round up to nearest kb

https://gerrit.wikimedia.org/r/663615

Change 663615 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Round up to nearest kb

https://gerrit.wikimedia.org/r/663615

^ This will make warnings less frequent which is an improvement. Is there a way we can automatically check if the size drops too far, e.g. can you set a lower limit in bundlesize?

There is no minSize currently.

There are several issues upstream that talk about similar problems:
https://github.com/siddharthkp/bundlesize/issues/365
https://github.com/siddharthkp/bundlesize/issues/271
https://github.com/siddharthkp/bundlesize/issues/207

Am open to using a different library if someone can find an alternative or perhaps we can make a donation to get one of the above fixed?

Right now however this is not a focus for me or the team. The annoyance is a small price to pay for catching accidental spikes that might have a performant impact.

We're just comparing two float numbers. If we start compromising how we compare those due to upstream difficulties, maybe it's time we reconsider using npm for such trivial problem and reconsider @hashar's proposal to make this a little PHPUnit structure test instead where you can use a one line statement to compare them however you want.

This is now a PHPUnit test inside core (\MediaWiki\Tests\Structure\BundleSizeTest )

Jdlrobson renamed this task from Bundlesize should not be cause V-1 when size increases by a few bytes to Bundlesize test should be updated to warn rather than fail when insignificant bytes increase.Jan 12 2024, 6:05 PM