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

Code golf preact.js.gz size down by 53 B ⛳️ #1599

Merged
merged 14 commits into from
May 5, 2019
Merged

Code golf preact.js.gz size down by 53 B ⛳️ #1599

merged 14 commits into from
May 5, 2019

Conversation

jviide
Copy link
Contributor

@jviide jviide commented May 4, 2019

This pull request applies a bunch of small code modifications that seem to reduce the overall bundle size. Here's a rough outline of the changes:

  • 276850e, 5e78b34, 37ce351, 07b42de mostly move around loop invariants to compress the code.
  • 3ec3829, d32aa17, 82f4cca reuse local variables etc. to avoid repetition.
  • 3f71109 replaces an Array#map call with Array#some that:
    1. should be just as supported as Array#map but doesn't collect all callback results to an array like Array#map
    2. seems to compress better than Array#forEach
  • 76529b7 reorganizes createContext extensively, getting rid of local variable and function declarations.
  • 7e53dd2 removes a superfluous check from diffProps - newProps's falsyness is checked although that shouldn't be possible at that point.
  • 04646b6 doesn't explictly init isNew to false, as its default undefined value is sufficient.
  • The rest of the commits mostly reorder code or modify if (x!=null) checks to if (x) when appropriate.

The performance seems to stay on the same level with master. I used Chrome 74, MacOS 10.14.4 and https://github.com/mathieuancelin/js-repaint-perfs for testing, so YMMV.

The size impact compared to the master is:

bundle file master random-opts delta
preact.js.gz 3548 B 3495 B -53 B
preact.js.br 3242 B 3201 B -41 B
preact.module.js.gz 3566 B 3517 B -49 B
preact.module.js.br 3265 B 3224 B -41 B
preact.umd.js.gz 3604 B 3561 B -43 B
preact.umd.js.br 3293 B 3255 B -38 B

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

All I can say is Wow :D

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

This PR is just... holy moly! I'd love to have @developit take a look at .some() vs the for loops. In my microbenchmarks (I may be doing them wrong) they are faster, but our lord of the benches can likely clear that one up :)

@coveralls
Copy link

coveralls commented May 5, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling fa52fc6 on jviide:random-opts into dc4ad8d on developit:master.

@jviide jviide changed the title Code golf preact.js.gz size down by 50 B ⛳️ Code golf preact.js.gz size down by 53 B ⛳️ May 5, 2019
@jviide
Copy link
Contributor Author

jviide commented May 5, 2019

Rebased the code on master, removed a couple of commits that used Array#some to replace for-loops (they seemed to bring down the performance a bit). Updated the pull request description accordingly.

@kristoferbaxter
Copy link
Collaborator

Accidentally closed from mobile GitHub. Sorry!

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

The benches were settled in slack 🥇 💯 This PR is really awesome! Starting from the detailed commits, to all the benches and all. It's just on a whole other level 👍 👍

@marvinhagemeister marvinhagemeister merged commit 8444dd4 into preactjs:master May 5, 2019
@jviide jviide deleted the random-opts branch May 5, 2019 09:14
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

5 participants