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

Monorepo -> Tree Shaking Refactor: Analyze Bundler Output Code #3459

Open
holgerd77 opened this issue Jun 17, 2024 · 13 comments
Open

Monorepo -> Tree Shaking Refactor: Analyze Bundler Output Code #3459

holgerd77 opened this issue Jun 17, 2024 · 13 comments

Comments

@holgerd77
Copy link
Member

holgerd77 commented Jun 17, 2024

Part of #3446

Earliest start together with official breaking release work!

When analyzing the code produced with esbuild respectively - to name it simpler - just scrolling through the produced code, e.g. when building the blockchain example from [here](npm run build && esbuild --bundle 08Blockchain.mjs --platform=browser --outfile=out1.mjs --format=esm) with npm run build && esbuild --bundle 08Blockchain.mjs --platform=browser --outfile=out1.mjs --format=esm it is totally mindblowing to see what's all in there together with the intuition/suspicion one is imminently getting that the very very most parts of this code will never be applied! 😋

We should definitely leverage this as well as an approach, this gives a very direct and clear feeling for the good, the bad and the ugly (for Blockchain I have e.g. discovered that the full Node.js Buffer code is still included there, still scratching my head about this 🤔).

To get some feeling, output code from these generated files look like this:

grafik

Or another example:

grafik

My first-round impression is there is a lot of room for improvement, also pointing to things going beyond the tree shaking topic.

@acolytec3
Copy link
Contributor

@roninjin10 question for you on tree-shaking. I've been doing some experiments with export maps that specifically define all of the entry points for a given module (specifically with statemanager as a starting point). When I define an export map in package.json as something like:

"exports: {
  './statemanager': {
"import": "dist/esm/stateManager.js"
},
'./rpcStateManager': {
"import": "dist/esm/rpcStateManager.js"
}
...

and take out the root export, vite seems to be very successful at excluding both internal code from statemanager as well as it's dependency tree when I bundle statemanager in a react app, like this:
image

vs like this when I have a single entry point '.': { "import": "dist/esm/index.js"}
image

In the second instance, only the unused code within the statemanager package is excluded while a whole bundle of transitive deps from the statemanager module that aren't used by the specific import in my react app (i.e. simpleStateManager.js) get bundled.

Do you like export maps as the main way of specifying entry points in modules to assist with tree-shaking? I haven't tested with other bundlers/frameworks but this seemed like it worked really really well and maybe should be our goal with the next round of breaking releases (i.e. to remove the default entry point on all of our downstream libraries and specify named entry points to reduce the overall bundle size for consuming apps).

@roninjin10
Copy link
Collaborator

TLDR

For the most part, No, export maps do not affect tree shaking.

Example

If I am an end user using webpack vite etc. and my bundler sees this:

import { rpcStateManager } from '@ethereumjs/state-manager'

It will step into that file

// index.js
export { rpcStateManager } from './rpcStateManager.js'
export {stateManager} from './statemanager.js'

and then it will immediately tree shake it

// index.js
export { rpcStateManager } from './rpcStateManager.js'
// remove unused export
// export {stateManager} from './stateManager.js'

After it tree shakes it the bundle hit will be the same size as it is when you use exports to seperate.

Exception

There are edge cases. For a UI like metamask that uses lavamoat, lavamoat doesn't support tree shaking. If a user isn't tree shaking themselves these seperate entrypoints are useful in this edge case because you essentially tree shaked for them.

Testing this

If you want to test how it looks like tree shaking what you can do is create a new file treeShakingTest.ts

import {rpcStateManager} from './index.ts'

Change that bundle analyzer to use this as the entrypoint and you will see how it will behave in this situation. Pointing it at index.js is the equivelent to doing this

import * as stateManagerModule from '@ethereumjs/state-manager'

Also I really like this online tool for doing this: .

image

@holgerd77
Copy link
Member Author

Ok, but guess we really need to have a close look why this didn't work with Andrews experiments, if I understood correctly he more or less did the same thing (do a file with a specific import and then check the resulting bundle size).

At the moment I perceive these results as contradictory and not giving a clear direction yet. 🤔

@roninjin10
Copy link
Collaborator

I can look. Could be a bunch of things preventing tree shaking.

Are side effects marked as false? https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free

@roninjin10
Copy link
Collaborator

roninjin10 commented Jul 9, 2024

Gave it a look and the reason tree shaking isn’t working in state manager package is this part of package.json

"type": "commonjs",

Change this to module and it should work. Cjs doesn’t tree shake

@holgerd77
Copy link
Member Author

@roninjin10 ah, yeah, that's very much on the plate/horizon anyhow to do the full switch and also go ESM for our internal setup, that's somewhat of a left-over! Guess we should put this very much of the front of the breaking release work, this was in the way of various stuff already and making things more complicated than it should be!

@acolytec3
Copy link
Contributor

Gave it a look and the reason tree shaking isn’t working in state manager package is this part of package.json

"type": "commonjs",

Change this to module and it should work. Cjs doesn’t tree shake

I'm not sure this is applicable. Our build process produces ESM and CJS outputs. I tried the same process I outlined above of switching statemanager to use an export map with named exports with webpack and it produced similar results to vite where it excluded whole dependency subtrees for the named exports that weren't imported by the consuming application so this feels like it will greatly enhance tree shaking for users of our libraries.

@roninjin10
Copy link
Collaborator

Trust me on this. I say this with 100% confidence export maps do not affect tree shaking and if it looks like they do that means tree shaking failed to happen.

This can be tested using a library that has export maps like viem

@roninjin10
Copy link
Collaborator

Tested this in a few instances to prove it. Setup here is a vite app that gets created with create wagmi

For zustand importing createStore from entyrpoint or vanilla are same
image

Same is true for a couple of other libraries I tested except for viem which showed a small but minimal difference of 0.2kb that didn't get tree shaken from entrypoint import

Our build process produces ESM and CJS outputs.

This could be true. Adding a console.log to the ESM imports only and then running the code is easiest way to test that it's properly using ESM. If it's is properly using ESM double check that "sideEffect": false is set in package.json

@roninjin10
Copy link
Collaborator

Testing tree shaking with Tevm I noticed ethereumjs tree shakes way differently depending on how aggressive or loose the tree shaking settings are set

Below we see that if I set tree shaking to smallest it tree shakes all JS while if I set it to recomended it includes almost all ethereumjs code.

This implies that some rule that "smallest" doesn't follow is the root cause. Docs about what rules smallest ignores are here. SideEffect: false in package.json would be my first guess

telegram-cloud-photo-size-1-4933710584994704417-y

@roninjin10
Copy link
Collaborator

The above code is using @ethereumjs/util as a sub dep of @tevm/utils I forgot to mention

@holgerd77 holgerd77 changed the title Monorepo -> Tree Shaking Refactor: Analyze esbuild Output Code Monorepo -> Tree Shaking Refactor: Analyze Bundler Output Code Sep 20, 2024
@holgerd77
Copy link
Member Author

Maybe not directly closing this, since this might still be worth to have some closer looks.

Just a technical update, since we are mostly bundling with vite/rollup now:

One can produce a readable bundle by adding minify: false as a build option to vite.config.bundler.ts.

@holgerd77
Copy link
Member Author

Also just have created this gist just for fun 🙂:
https://gist.github.com/holgerd77/2c032488196b4afee5d976dc85ee70eb

(if someone wants to have a look without doing the above steps)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants