try to speed up copyPages

cantoo-scribe / pdf-lib

Create and modify PDF documents in any JavaScript environment

https://pdf-lib.js.org

MIT License

136 stars 27 forks source link

try to speed up copyPages #48

Closed acestronautical closed 8 months ago

acestronautical commented 8 months ago

What?

Change copyPages to use promises because merging PDFs is slow

Why?

merging PDFs is slow

How?

using promises

Testing?

I tested it in my own project and the page ordering seem to stay the same, I also ran the tests.

New Dependencies?

Screenshots

dcbr commented 8 months ago

~How does making it an async function actually speed up the copying, do you have some timings? You would still need to await this function before being able to actually do something with the copied pages, which also makes it quite dangerous/tricky if this is not properly done I assume.~

Edit: Nevermind, I had misinterpreted the changes, looks ok; sorry for the noise!

acestronautical commented 8 months ago

Sorry people keep telling me to amend this to different things. I had modified to @Sharcoux version but I think it needed to be return await Promise.all(copiedPages); otherwise as @dcbr said we would have to await the function.

This speeds up the copying of pages because it allows concurrency/parallelism of each page being copied rather than having to do them iteratively.

I don't have actual timings, just my subjective experience that the modified version seems to make bookbinder-js faster.

Sharcoux commented 8 months ago

@acestronautical Sorry, I made this wait a bit. I'm back on it now. I'll go with the implementation of @MatheusrdSantos and close this PR.

The fix has been released under v1.20.4

Thanks to you all!