brownplt / code.pyret.org

Website for serving Pyret to folks.
Other
24 stars 45 forks source link

Performance: make cpo-main smaller by trimming unneeded sourcemaps and minifying compiled code #397

Closed asolove closed 1 year ago

asolove commented 3 years ago

⚠️ This is not ready to be merged. It currently runs over all compiled modules, when it should only run over compiler internals. Adam is adding this PR to start discussion and to see if tests break.

See the lengthy description inline in the code.

On my machine, this reduces the unminified code from 32mb to 19mb, and the gzipped version from 5.3mb to 3.7mb. (This difference will get slightly smaller when we add back in sourcemaps for userland libraries, but not much. The vast majority of the code is internals.)

This builds on top of the work in #396 to minify cpo-main, which needs to land first. So changes for discussion here start at 58dcaad.

asolove commented 3 years ago

Hahaha the Travis build is out of memory.

A couple next steps:

jpolitz commented 3 years ago

We could ask for bigger Travis instances, though currently we're on the “ask for OSS credits” tier, so that may or may not work. If we know ballpark what we should ask for, I'll ask (they are super responsive). We do have a budget, though it's not bottomless, for stuff like this; you shouldn't pay for the org.

Separately, we already pay for Github, and a potential TODO is to try it on Github Actions. I like Travis, but if we're already paying for an org repo with an education discount, that might be covered.

asolove commented 3 years ago

I'll try code changes here first and make sure we really need it before we ask for something like that.

FWIW, I think we can ask for large Travis instances just via the yaml config, but then we'll burn through the open source credits twice as fast.

asolove commented 3 years ago

NB: pushed a rebase on the upstream branch to clean up the diff and retry CI. Also pushed an alternate version that cleans up sourcemaps but doesn't inline js, to see if that will work within current CI limits.

jpolitz commented 3 years ago

A thought: the compiler leaves compiled versions of each file in compiled/<name>.arr-<sha>-module.js. These could be individually minified, then a compiler rebuild would use those versions rather than the full versions. These are stitched together into the standalone.

Without doing any compiler hacking, it should be possible to minify these individually, then remove and rebuild cpo-main.jarr without touching the .arr source files, and the minified versions would get stitched together instead.

You could have a separate rule for making cpo-main.jarr the second time that uses a different compiled directory that you manually construct from these minified files (e.g. like the $(CPOMAIN) rule but with --compiled-dir ./minified-compiled). That might be an avenue to pursue.

asolove commented 3 years ago

Yeah, I like that!

On Mon, Aug 23, 2021 at 1:40 PM Joe Politz @.***> wrote:

A thought: the compiler leaves compiled versions of each file in compiled/.arr--module.js. These could be individually minified, then a compiler rebuild would use those versions rather than the full versions. These are stitched together into the standalone.

Without doing any compiler hacking, it should be possible to minify these individually, then remove and rebuild cpo-main.jarr without touching the .arr source files, and the minified versions would get stitched together instead.

You could have a separate rule for making cpo-main.jarr the second time that uses a different compiled directory that you manually construct from these minified files (e.g. like the $(CPOMAIN) rule but with --compiled-dir ./minified-compiled. That might be an avenue to pursue.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/brownplt/code.pyret.org/pull/397#issuecomment-903977272, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACCL6LYD4IZLEGADMCYWDT6KBZLANCNFSM5COGJYGQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

asolove commented 3 years ago

So two interesting things happened here when I pulled out the sourcemaps but didn't inline the js

  1. The tests passed, even though I removed all sourcemaps for all modules. This makes me concerned the tests aren't enough to notice if I broke some error messages.
  2. The gzipped version of the file is only slightly larger than with all the js minification (3.7mb compared to 3.4mb, whereas before this PR it was >5mb)

Which makes me think that:

  1. I want to manually check what removing the sourcemaps does and whether any error messages were impacted
  2. If I can sort that out, it would be worth landing just the sourcemap removal before worrying about the per-module minification.
blerner commented 3 years ago

I don't think that any of our tests specifically check for the rendering of the call-stacks in any error cases; I think they just check that the error rendered successfully. The stack trace itself is hidden behind the "Show program evaluation trace" link in the error message. Stack traces aren't usually useful in the automated tests; they've historically been more helpful for us debugging goofs in the compiler...

jpolitz commented 3 years ago

Yeah, I need to figure out how to test this. I was hoping something would break if they were gone because we use top-of-stack for some of the function call error renderings. But maybe all the existing tests that do that all use the maps generated from user code.

@asolove easiest may be to add something to test-util.js that checks for some sensible srcloc predicate over the stack trace of some built-in error.

schanzer commented 1 year ago

@asolove this PR has definitely gone stale. Wondering if there are plans to resurrect it, or if it's moot because of Anchor?

asolove commented 1 year ago

Ah yeah, this is now very old. There is still plenty of opportunity to save on bundle sizes in both the CPO and the Anchor UIs with this strategy, but the details will be different.

@schanzer have you seen folks having issues with performance that it's worth reopening this work?

schanzer commented 1 year ago

No, I was just curious. The only performance bottlenecks that are really hurting us are operations when dealing with large tables. But my understanding is that there isn't much that can be done about that.