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

Tree shaking in split code #2788

Open
skeggse opened this issue Apr 2, 2019 · 14 comments
Open

Tree shaking in split code #2788

skeggse opened this issue Apr 2, 2019 · 14 comments

Comments

@skeggse
Copy link
Contributor

skeggse commented Apr 2, 2019

Expected Behavior / Situation

// one.js
import {forOne} from './three';
forOne();

// two.js
import {forTwo} from './three';
forTwo();

// three.js
export function forOne() {
  console.log('abc');
}

export function forTwo() {
  console.log('def');
}

// rollup.config.js
export default {
  input: {
    one: './one.js',
    two: './two.js',
  },
  output: {
    dir: './dist/',
    format: 'esm',
  }
};

Results in two chunks:

// dist/one.js
function forOne() {
  console.log('lol');
}

forOne();

// dist/two.js
import { b as forTwo } from './chunk-120c4305.js';
  
forTwo();

Actual Behavior / Situation

The above results in three chunks:

// dist/one.js
import { a as forOne } from './chunk-120c4305.js';
  
forOne();

// dist/two.js
import { b as forTwo } from './chunk-120c4305.js';
  
forTwo();

// dist/chunk-120c4305.js
function forOne() {
  console.log('abc');
}

function forTwo() {
  console.log('def');
}

export { forOne as a, forTwo as b };

Modification Proposal

Handy-wavy explanation: have each entry module or manual chunk perform tree-shaking to only include code used by their execution graphs.

@lukastaegert
Copy link
Member

lukastaegert commented Apr 3, 2019

Thanks, this issue is quite interesting. My first impulse was to dismiss it as "this is not how it works" etc. but on further thought you really have an interesting point and the approach could actually work. Nevertheless it will be a long term improvement as it will touch some fundamental things.

But a few things first: At the moment, code-splitting always produces a single module graph and this is how it will be for the foreseeable future as the goal is not to create standalone bundles but share as much as possible, but I guess you are aware of that.

The problem is that the finest granularity that code-splitting has at the moment is at the module-level, i.e. parts of a module cannot end up in different chunks, which is why Rollup produces the output it does.

Now expanding upon your idea, this is what we could do:

  • Run tree-shaking for each entry point separately. Tree-shaking puts markers on nodes telling us which nodes are included. For this to work, top level nodes will need to receive different markers depending on the entry points that include them.
  • Side-effect detection on the other hand should not distinguish between entry points—e.g. mutating a variable that is only used by a different entry point should still count as a side-effect.
  • Once everything is marked, we split up the module into "submodules" depending on which entry points depend on which nodes. The difficult part here will be to figure out how to handle the additional imports/exports needed that do not exist in the original code. Maybe we can rework this complete logic and even simplify it. As both imports and exports of chunks are generated by Rollup and do not depend on existing nodes, this should be possible.
  • Now we run the existing chunking algorithm but using the submodules instead of the original modules. In your example, this is what the output would be:
// dist/one.js
function forOne() {
  console.log('abc');
}

forOne();

// dist/two.js
function forTwo() {
  console.log('dev');
}

forTwo();

so no shared chunk at all. If we add a third shared function

// one.js
import {forOne, forBoth} from './three';
forOne();
forBoth();

// two.js
import {forTwo, forBoth} from './three';
forTwo();
forBoth();

// three.js
export function forOne() {
  console.log('abc');
}

export function forTwo() {
  console.log('def');
}

export function forBoth() {
  console.log('efg');
}

then this would generate a shared chunk containing only the shared function:

// dist/one.js
import { a as forBoth } from './chunk-120c4305.js';

function forOne() {
  console.log('abc');
}

forOne();
forBoth();

// dist/two.js
import { a as forBoth } from './chunk-120c4305.js';

function forTwo() {
  console.log('dev');
}

forTwo();
forBoth();

// dist/chunk-120c4305.js
function forBoth() {
  console.log('efg');
}

export { forBoth as a};

So how close are we to this? To be honest, it will be a lot of work, but it would be worthwhile in the way that this is something that to my knowledge no other bundler can do. Nevertheless before starting on this, I would at least love to have the open points of #2407 fixed.

@localvoid
Copy link

this is something that to my knowledge no other bundler can do

Cross-module code motion in google closure compiler.

@fengxinming

This comment has been minimized.

@lukastaegert

This comment has been minimized.

@jakearchibald
Copy link
Contributor

Mostly a dupe: #3017

But a 'quick' solution might be a way to say "please don't code-split 'three.js', always bundle it".

@lukastaegert
Copy link
Member

If you just want to reduce the number of chunks, a powerful tool you can already use today are manualChunks. For instance for your three.js example, if you put three.js into a manual chunk it becomes unsplittable. Unless another manual chunk interferes.

@jakearchibald
Copy link
Contributor

Wouldn't that force three.js into a chunk? Whereas what we'd need is the opposite of that: "Don't put 'three.js' into its own chunk, bundle it into both one.js and two.js".

@lukastaegert
Copy link
Member

But then you would have two copies in the graph, which is something that Rollup will never do due to a bunch of reasons, code duplication only being one of them? You could also put either one.js or two.js into the same chunk as three.js so that only one of the chunks has a dependency.

@jakearchibald
Copy link
Contributor

jakearchibald commented Aug 5, 2019

something that Rollup will never do due to a bunch of reasons, code duplication only being one of them?

Hmm yeah, I guess if both were used in the same realm, anything that depended on state or object equality would totally break.

Fwiw, my expectations here were led by code like this:

main.js

import { foo } from './sub.js';
if (foo) {
  console.log('hi');
}

main2.js

import { bar } from './sub.js';
if (bar) {
  console.log('hi');
}

sub.js

export const foo = true;
export const bar = false;

Which transpiles to:

main.js

'use strict';
{
  console.log('hi');
}

main2.js

'use strict';

I guess this is special-cased in Rollup, and I thought that other primitive types might behave the same.

Anyway I agree that #2788 (comment) is a better solution, as it works for all types.

@jakearchibald
Copy link
Contributor

To give a real-world case, I have code like:

import version from 'consts:version';

Which (via a plugin) is:

export default '1.2.3';

I was kinda hoping that string would be rolled into each module that uses it, rather than it becoming a seperate chunk. I guess I should be using https://github.com/rollup/rollup-plugin-replace instead, but I liked the explicit nature of the import.

@lukastaegert
Copy link
Member

Possibly a way to handle it, but this is only a good idea if the string is not 1000 characters long. Hard to know where you would want to draw the line.

@jakearchibald
Copy link
Contributor

Yeah, I guess you'd have to punt it to an option. Maybe it's just too complicated to be generally useful.

@shellscape
Copy link
Contributor

Hey folks. This is a saved-form message, but rest assured we mean every word. The Rollup team is attempting to clean up the Issues backlog in the hopes that the active and still-needed, still-relevant issues bubble up to the surface. With that, we're closing issues that have been open for an eon or two, and have gone stale like pirate hard-tack without activity.

We really appreciate the folks have taken the time to open and comment on this issue. Please don't confuse this closure with us not caring or dismissing your issue, feature request, discussion, or report. The issue will still be here, just in a closed state. If the issue pertains to a bug, please re-test for the bug on the latest version of Rollup and if present, please tag @shellscape and request a re-open, and we'll be happy to oblige.

@lukastaegert
Copy link
Member

I would like to keep this one for reference as it is one of two issues that describe two competing approaches how to do sub-module-level code-splitting. This is something we definitely want at some point.

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

6 participants