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.

Sign up for GitHub

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

Jump to bottom

Fix PNG memory leak #862

Closed
wants to merge 24 commits into from
Closed

Fix PNG memory leak #862

wants to merge 24 commits into from

Conversation

rsshilli
Copy link

@rsshilli rsshilli commented Aug 24, 2018 •
edited
Loading

This is to fix for #861. I created it on top of @blikblum's very awesome convert-es6 branch that he created for #859 so please merge that first.

CorvusCorrax and others added 24 commits September 8, 2017 10:29
@CorvusCorrax
remove stream extension from reference
6ccaaff
@CorvusCorrax
Fix font
f19b455
@CorvusCorrax
Use buffer instead of multiple writes
fd4392d
@CorvusCorrax
use buffer concat only once
98c4489
@blikblum
Create embed tag name using a deterministic algorithm based on font id
797a160
@blikblum
Add integration tests (check for generated pdf). Covers examples dist…
569b53f 
…ributed by pdfkit and pdfmake
@blikblum
Add tests for tables, watermark and qrcode pdfmake demos
d75fdea
@blikblum
Remove tests dir from .gitignore
9e5b9a3
@blikblum
Implement saving pdf files when test snapshot is updated/added. Write…
8e1aae2 
…s a pdf with the difference if snapshot does not matches
@blikblum
Do not compress pages
e101e47
@blikblum
Fix removing changed pdf
9d7a1b7
@blikblum
Add eslint dependency
81e1dd8
@blikblum
Extract PDFFont.open class method to PDFFontFactory to break circular…
aac4065 
… dependency on font classes
@blikblum
Break circular reference between PDFObject and PDFReference
716ae74
@blikblum
Merge memory leak fixes (PR foliojs#729)
fed0e5c
@blikblum
Update test snapshots
0449871
@blikblum
decaffeinate: Rename abstract_reference.coffee and 24 other files fro…
9067903 
…m .coffee to .js
@blikblum
decaffeinate: Convert abstract_reference.coffee and 24 other files to JS
d4f32d2
@blikblum
decaffeinate: Run post-processing cleanups on abstract_reference.coff…
2018e2b 
…ee and 24 other files
@blikblum
Fix import issues and remove constructor hack
b4dba45
@blikblum
Do not try to set length property of reference buffer (Buffer instance)
e61498f
@blikblum
Add missing var in svg test
20e3f19
@blikblum
Add dependencies and script to build
ba902b0
@rsshilli
foliojs#861 Fixing a memory leak when adding too many pngs too quickly.
513f609
@blikblum
Copy link
Member

Some remarks:

  • Can you test replacing zlib.deflate by zlib.deflateSync?
  • The es6 convert branch is a moving target. Currently i'm expanding tests so can continue the conversion
  • In process of removing deccafeinate artifacts i removed some code that impacts performance and also identified some low hang fruits to reduce memory usage, so when conversion is finished there should be some perf gain

@blikblum
Copy link
Member

I created a build using deflateSync

You can test by using pdfkit-es@pngsync version

@rsshilli
Copy link
Author

rsshilli commented Sep 20, 2018 •
edited
Loading

I haven't tried it and honestly the next few weeks are a big release so I doubt I'll get to it soon. But this looks like a good solution. I don't see why deflateSync wouldn't work.

@blikblum
Copy link
Member

Probably fixed by #889 . The issue still can occur because png-js decodePixels calls zlib inflate async, but odds are much smaller.

@blikblum blikblum closed this Dec 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rsshilli @blikblum @CorvusCorrax