-
-
Notifications
You must be signed in to change notification settings - Fork 504
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: function calls on module instantiation are not removed #1087
Comments
Keep in mind that this: -var contramap = pipeable(ord).contramap;
+var pipeableResult = /*#__PURE__*/pipeable(ord);
+var contramap = pipeableResult.contramap; won't really help much, because |
I discovered that this is because webpack is not as aggressive as Rollup: webpack/webpack#10253 (comment) In turn, this is because Terser does not respect "pure" comments on getters: terser/terser#513. (As pointed out by @Andarist above.) Therefore we would need to do this: diff --git a/node_modules/fp-ts/es6/Ord.js b/node_modules/fp-ts/es6/Ord.js
index c6d925d..438bf26 100644
--- a/node_modules/fp-ts/es6/Ord.js
+++ b/node_modules/fp-ts/es6/Ord.js
@@ -137,8 +137,8 @@ export function getSemigroup() {
}
const M = {
// tslint:disable-next-line: deprecation
- concat: getSemigroup().concat,
- empty: fromCompare(() => 0)
+ concat: /*#__PURE__*/(() => getSemigroup().concat)(),
+ empty: /*#__PURE__*/fromCompare(() => 0)
};
/**
* Returns a `Monoid` such that:
@@ -245,7 +245,8 @@ export const ord = {
URI,
contramap: (fa, f) => fromCompare((x, y) => fa.compare(f(x), f(y)))
};
-const { contramap } = pipeable(ord);
+const pipeableResult = /*#__PURE__*/pipeable(ord);
+const contramap = /*#__PURE__*/(() => pipeableResult.contramap)();
export {
/**
* @since 2.0.0
@@ -254,4 +255,4 @@ contramap };
/**
* @since 2.0.0
*/
-export const ordDate = ord.contramap(ordNumber, date => date.valueOf());
+export const ordDate = /*#__PURE__*/ord.contramap(ordNumber, date => date.valueOf()); Here is a reduced test case where you can see how tree shaking works in both Rollup and webpack: https://github.com/OliverJAsh/tree-shaking-test/tree/fp-ts-issue-1087. If you run @gcanti What do you think about making these changes?
|
This by @Andarist might help: https://github.com/Andarist/babel-plugin-annotate-pure-calls |
|
Good question. I don't have an immediate answer, but I'll give it some thought. The reason I ended up going down this rabbit hole is because I noticed that we were bundling more code than we actually use, e.g. our bundle contains |
It's not only about pipe being called but also about arguments passed to it. If everything is wrapped in pipe and pipe is not marked as pure then it, in turn, holds to everything and nothing can be tree-shaked. The very similar situation was in Ramda - everything is curried, but by making similar changes some time ago I was able to make tree-shaking work for it 100%. If you import a single function from Ramda (since 0.25) then you will end up only with that single function and with what it depends on. |
If fp-ts wanted to achieve this, I think we'd have to remove |
Oh, I've looked into what |
@Andarist I remember there was a period of hacks and Rollup-plugins for making Ramda including builds smaller: |
There was, but such hacks are no longer needed for ramda. IMHO would be better to fix this idiomatically here rather than introduce custom hacks like this. |
I have been working on making tree shaking work for the modules I use in matechs-effect, I can confirm that pipeable is a very problematic component. Basically as noticed before having pipeable make the whole module non shakable, furthermore the sideEffect flag is only on a module level so whenever we include any function from a module we are including the full module. I have changed the module structure to isolate each function in a single file basically one module per function and shaking is happly working. One other thing that is important is how packages are published currently we have a dual /lib /es6 and this is not optimal, having a material-ui like publish mechanism where in each sub-module there is a package.json listing the es6 variant in it is better because the same code (even if required by a library) gets propertly indexed for the shaking algorithms, basically imports will have to look like "fp-ts/Array" where Array has itself a package.json so even if "fp-ts/Array" is required by a dependency we don't end up with the es5 version. Slighlty unrelated (more an opinion) large typeclasses are also an enemy of shakability, for example option has a wide number of instances in it while for shakability is preferred to have variants with only for example Applicative or Monad like: I am doing this for the modules listed at: In case we decide to persue this for the core fp-ts I would be able to backport those here, the change will unfortunately be breaking because of the module structure) -- |
@OliverJAsh I would give it a try, starting from handling So, for example, in const {
alt,
ap,
apFirst,
apSecond,
chain,
chainFirst,
duplicate,
extend,
filter,
filterMap,
flatten,
foldMap,
map,
partition,
partitionMap,
reduce,
reduceRight,
compact,
separate,
fromEither
} = pipeable(option) ...will be replaced by const pipeables = /*#__PURE__*/ pipeable(option)
const alt = /*#__PURE__*/ (() => pipeables.alt)()
const ap = /*#__PURE__*/ (() => pipeables.ap)()
const apFirst = /*#__PURE__*/ (() => pipeables.apFirst)()
const apSecond = /*#__PURE__*/ (() => pipeables.apSecond)()
const chain = /*#__PURE__*/ (() => pipeables.chain)()
const chainFirst = /*#__PURE__*/ (() => pipeables.chainFirst)()
const duplicate = /*#__PURE__*/ (() => pipeables.duplicate)()
const extend = /*#__PURE__*/ (() => pipeables.extend)()
const filter = /*#__PURE__*/ (() => pipeables.filter)()
const filterMap = /*#__PURE__*/ (() => pipeables.filterMap)()
const flatten = /*#__PURE__*/ (() => pipeables.flatten)()
const foldMap = /*#__PURE__*/ (() => pipeables.foldMap)()
const map = /*#__PURE__*/ (() => pipeables.map)()
const partition = /*#__PURE__*/ (() => pipeables.partition)()
const partitionMap = /*#__PURE__*/ (() => pipeables.partitionMap)()
const reduce = /*#__PURE__*/ (() => pipeables.reduce)()
const reduceRight = /*#__PURE__*/ (() => pipeables.reduceRight)()
const compact = /*#__PURE__*/ (() => pipeables.compact)()
const separate = /*#__PURE__*/ (() => pipeables.separate)()
const fromEither = /*#__PURE__*/ (() => pipeables.fromEither)() right?
If I put up a branch with such a changes, would you be able to check whether they are actually working? |
I can volunteer to checking out the changes |
👌
Absolutely! |
Another alternative would be to kill pipeable given it will still add a lot of additional stuff to the bundle, we should also expose the data-last functions to allow other modules to not depend on the full instance, a prototype PR with Either is: |
If I understand it correctly, pipeable could be left there for anyone not concerned with tree shaking to quickly implement pipeable instances, but not used internally |
@Andarist @OliverJAsh I put up a Here's the repo I'm using to test different snippets https://github.com/gcanti/tree-shaking-test Some examples: Array snippet rollup:
webpack
TaskEither snippet rollup:
webpack
Changes so far:
|
@gcanti Could you add the |
@OliverJAsh done |
That definitely fixes the issue I described in my original post. When I tried it on the Unsplash app, the total size of fp-ts in our bundle decreased from 19 KB to 17 KB gzipped. |
I can also confirm this improves things a little bit - the only "big" thing that could be done to improve the situation more is removing |
I would go for that, I even suggested to move to curried data-last typeclasses in 3.0. |
That should be carefully evaluate I am the opinion that it will have severe performance implications. I can confirm removing pipeable improves significantly shakability (like also removing usage of transformers). Another step would be to skin the typeclasses, instead of having 1 that bundles all toghether we might have many smaller to be used ad hoc in combinators, for examle instead of one implementing |
I'm not sure about that. Most of the time we use curried data-last top-level functions which are produced by |
@raveclassic I did a test a while ago making the base of matechs-effect curried and I was in fact paying a good 2x - probably it can still be worked around by not using the instance where perf count but we should do proper testing before. In respect of pipeable if you remove it you will not have the calls anyway regardless of the typeclass structure - anyway discussion for a different thread |
#1216 if anyone wants to chime in
AFAIK this can be done without breaking changes, it's just a lot of manual work |
@Andarist @OliverJAsh I removed Here's the results in my test repo (https://github.com/gcanti/tree-shaking-test) Array snippet rollup:
webpack:
TaskEither snippet rollup:
webpack:
Task snippet rollup:
webpack:
|
@gcanti Looks like it's missing the |
@OliverJAsh opsss... sorry (commented out the wrong line in .gitignore) should be ok now |
Thanks! Unfortunately I can't test this because it seems to have broken integration with https://github.com/devexperts/remote-data-ts Is there a quick patch I can make to that library to fix this? Then I will be able to run some tests. |
Looks like this isn't working for some reason: https://github.com/devexperts/remote-data-ts/blob/23a83624d0f72377c5467cf0a97d0c3ded5d68bf/src/remote-data.ts#L26-L30 |
@OliverJAsh weird, honestly I can't see how my refactoring could affect that library. Is there a way I can repro? I added a |
It's working now. 😕 False alarm. In our codebase:
It looks like our usage of fp-ts has changed since I last reported test results, and I can't remember which commit I tested it against last time. I'd be curious to test the changes without the removal of |
@OliverJAsh branch |
Thanks. I'm seeing exactly the same size for both |
@OliverJAsh ^ this is the case I guess. In a Here's the stats Array snippet rollup:
webpack:
TaskEither snippet rollup:
webpack:
Task snippet rollup:
webpack:
Ok thanks for the help, I'm going to release v2.6.2 |
Awesome! Thanks for your work on this. |
Closed by #1220 |
@OliverJAsh I'm working on v3 and there are some ideas that I would like to backport to v2 and see if they make any difference with respect to tree shaking
Note: top level |
Yaaay! If you need any help, please let me know! |
@raveclassic thanks, you can get a sneak pick on branch |
Closed by #1232 @OliverJAsh just released v2.6.3 could you please try it out? My import * as _ from "fp-ts/es6/TaskEither";
import { pipe } from "fp-ts/es6/pipeable";
pipe(
_.right(1),
_.map(n => n + 1),
_.chain(n => _.right(n + 1)),
_.swap
); went from 11K to 7K (rollup) Stats:
Most benefits should come from replacing all occurrences of |
|
@OliverJAsh nice, thank you! |
I know |
🐛 Bug report
Current Behavior
Reduced test case
src/index.js
:rollup.config.js
:package.json
:Then run
rollup -c rollup.config.js
.Expected behavior
The output of rollup should not include lots of unused code:
Actual behaviour
The output of rollup includes lots of unused code:
Show code snippet
Reproducible example
Suggested solution(s)
Add annotations/comments to indicate no side effects, e.g. in
fp-ts/es6/Ord.js
:This fixes the issue when using Rollup. Reduced test case.
Note this does not fix the issue with webpack, for some reason: webpack/webpack#10253
Additional context
Your environment
Which versions of fp-ts are affected by this issue? Did this work in previous versions of fp-ts?
The text was updated successfully, but these errors were encountered: