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 unneccessary async in embed #66

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nayounsang
Copy link

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

Suggested Reading?

Anything Else?

Checklist

  • I read CONTRIBUTING.md.
  • I read MAINTAINERSHIP.md#pull-requests.
  • I added/updated unit tests for my changes.
  • I added/updated integration tests for my changes.
  • I ran the integration tests.
  • I tested my changes in Node, Deno, and the browser.
  • 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!
  • My changes work for both new and existing PDF files.
  • I ran the linter on my changes.

@nayounsang nayounsang changed the title refactor: remove unneccessary asyncin embed refactor: remove unneccessary async in embed Sep 4, 2024
delete lock file from PR
@Sharcoux
Copy link
Collaborator

Sharcoux commented Sep 5, 2024

Please remove your changes to yarn and package-lock.

I'll have a look at the rest.

@@ -1070,7 +1065,7 @@ export default class PDFDocument {
async embedJpg(jpg: string | Uint8Array | ArrayBuffer): Promise<PDFImage> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this async should not be needed anymore either, right?

@@ -1110,7 +1105,7 @@ export default class PDFDocument {
async embedPng(png: string | Uint8Array | ArrayBuffer): Promise<PDFImage> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also be able to remove it from embedSvg. try to look for other impacts.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for review. I will check other async function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for review. Plz check for changes.

@nayounsang
Copy link
Author

Please remove your changes to yarn and package-lock.

I'll have a look at the rest.

Oh, this is my mistake

@nayounsang
Copy link
Author

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
Copy link
Collaborator

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.

@nayounsang
Copy link
Author

nayounsang commented Sep 11, 2024

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants