refactor: remove unneccessary async in embed

nayounsang commented 1 week ago

What?

I implement about this issue Some of the embedded code is async, but Inside the function is synchronous.

Why?

I had a bottleneck in that library and found the code while looking inside. This may cause confusion in its internal workings.

How?

Remove async at embed PNG,JEPG,Fonts. This is also reflected in where it is used.(PDF Document)

Testing?

I run test scripts. All passed. And run test apps/web/test-htmls. From my own experience, this works without a problem.

New Dependencies?

No

Screenshots

Anything Else?

Checklist

[x] I read CONTRIBUTING.md.
[x] I read MAINTAINERSHIP.md#pull-requests.
[x] I added/updated unit tests for my changes.
[x] I added/updated integration tests for my changes.
[x] I ran the integration tests.
[x] I tested my changes in Node, Deno, and the browser.
[x] I viewed documents produced with my changes in Adobe Acrobat, Foxit Reader, Firefox, and Chrome.
[ ] I added/updated doc comments for any new/modified public APIs. - API not changed!
[x] My changes work for both new and existing PDF files.
[x] I ran the linter on my changes.

Sharcoux commented 1 week ago

Please remove your changes to yarn and package-lock.

I'll have a look at the rest.

nayounsang commented 1 week ago

Please remove your changes to yarn and package-lock.

I'll have a look at the rest.

Oh, this is my mistake

nayounsang commented 1 week ago

And I found a lot of things that were just another async function but were actually sync functions. Embedding probably won't have any impact, but other things will need to be figured out more and worked on sequentially. If this library needs!

MatheusrdSantos commented 6 days ago

Hi, @nayounsang Those parts of the code are async for performance reasons. They were intentionally added here: https://github.com/Hopding/pdf-lib/commit/c18789c49889ab4896e32659b399dd3c08613adf and https://github.com/Hopding/pdf-lib/commit/cc00627d1c8738282bafd3e0d7e34180e2e33228

There's also an explanation here: https://github.com/Hopding/pdf-lib/issues/134#issuecomment-509214350. The explanation is related to the document create/load process, but it's also valid for the embedding process.

nayounsang commented 6 days ago

Hi, @nayounsang Those parts of the code are async for performance reasons. They were intentionally added here: Hopding@c18789c and Hopding@cc00627

There's also an explanation here: Hopding#134 (comment). The explanation is related to the document create/load process, but it's also valid for the embedding process.

I have a question of this comment. The inside of create/load/embed functions are all synchronous code. So from what I've learned, this has nothing to do with event loop blocking. For example

const myFn = async () => {
  let a = 10;
  for (int i = 0; i < 1000000000; i++){
    a += 1;
  }
}
await myFn() // nothing to do with event-loop

But I am still a student so I may be confused about the event loop. Can you explain more about this?

nayounsang commented 6 days ago

After asking js experts around me, they guessed that the maintainer probably used async with future scalability in mind. I also experienced huge CPU occupancy and calculation while using this, so I think it is important to manage the event loop. However, the bottom line is that pdf-lib may need to use techniques (for ex: worker threads) to make sure this doesn't really block the event loop.

cantoo-scribe / pdf-lib