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

refactor: remove ramda from layout package #1827

Merged
merged 1 commit into from
May 30, 2022

Conversation

diegomura
Copy link
Owner

Gradually start removing ramda. Although I like it, it's probably confusing for most people, it's slow and adds unneccesary bundle size. Some helper fns will still be needed. For the moment I moved them to utils dir, in the future it worths building our own set of light and reusable utils

@diegomura diegomura force-pushed the dm/remove-randa-from-layout branch from 349df13 to d1545e0 Compare May 25, 2022 05:54
@diegomura diegomura changed the title chore: remove rambda from layout package refactor: remove rambda from layout package May 25, 2022
@jeetiss
Copy link
Collaborator

jeetiss commented May 25, 2022

I made a benchmark and the result is insane, almost 20% speed up on a simple document with one page.

master:

yarn run v1.22.17
$ babel-node ./bench/index.jsx

Running "simple" suite...
generate doc x 33 ops/sec ±7.7% (60 runs sampled)
✨  Done in 8.22s.

your branch:

yarn run v1.22.17
$ babel-node ./bench/index.jsx

Running "simple" suite...
generate doc x 40 ops/sec ±8.9% (63 runs sampled)
✨  Done in 7.80s.

@diegomura
Copy link
Owner Author

Wow that's great! Thanks for checking @jeetiss !

In addition to make code more vanilla that's the main motivation behind this effort, glad the benefits are visible. Using Object.assign instead of spread and small changes like that are aligned to this as well.

And that's not even considering ramda still dominates the textkit, stylesheet and render packages (textkit gains specifically should be big, considering text layout is one of the most expensive pieces here). So expect more improvements!

@diegomura diegomura changed the title refactor: remove rambda from layout package refactor: remove ramda from layout package May 25, 2022
@diegomura diegomura self-assigned this May 26, 2022
@diegomura
Copy link
Owner Author

I should be done removing ramda everywhere in some few days more of work. There's an open PR to remove it completely from @react-pdf/stylesheet. Still to go some cases in @react-pdf/layout (this PR), @react-pdf/textkit (PR in progress, by far where we use it the most, which sounds promising), and @react-pdf/render (no PR nor progress yet)

@diegomura diegomura force-pushed the dm/remove-randa-from-layout branch from d1545e0 to eb21029 Compare May 28, 2022 08:05
@diegomura
Copy link
Owner Author

diegomura commented May 28, 2022

@jeetiss with upstream changes now benchmarks look even better in this branch 😄

And there are still packages to migrate. It's a tedious work but most if it it's done already

generate doc x 89 ops/sec ±4.84% (76 runs sampled)
✨  Done in 8.57s.

@diegomura diegomura force-pushed the dm/remove-randa-from-layout branch from eb21029 to e386694 Compare May 29, 2022 20:26
@diegomura diegomura force-pushed the dm/remove-randa-from-layout branch from e386694 to 4da728e Compare May 30, 2022 03:55
@diegomura diegomura marked this pull request as ready for review May 30, 2022 03:57
@diegomura diegomura merged commit 7c1d373 into master May 30, 2022
@diegomura diegomura deleted the dm/remove-randa-from-layout branch May 30, 2022 04:01
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.

2 participants