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

Write images in buffers #1

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

Write images in buffers #1

wants to merge 2 commits into from

Conversation

naggingant
Copy link
Member

@naggingant naggingant commented Jan 18, 2021

@openrm/dev

This is an attempt to register/write the image streams as they are, whenever possible.
It is done by replacing the existing redundant memory copies (full reads) with byte-wise operations, and instead doing it only once for adding the Length metadata on each object.

All tests are passing, and I tested it in the current PDF service of ours also.

@PuKoren
Copy link
Member

PuKoren commented Jan 18, 2021

I'll check it tomorrow, I need a fresh brain for this one 😰

@naggingant
Copy link
Member Author

naggingant commented Jan 18, 2021

I feel too bad to ask for a review, so I will first try to describe what is going on. Give me a minute, please.

(Wrote this last year, in my free time and I need some time to remember everything myself...)

@PuKoren
Copy link
Member

PuKoren commented Jan 18, 2021

ha I see, thanks! also I would be interested to know about the performance gains, if it does not take you too much time

@naggingant
Copy link
Member Author

Quickly ran benchmarks, and basically it is not always "better", and I remembered this was the primary reason I did not open this PR first 😅

The collected benchmark results and profiles were quite hard to understand at the moment, but I'll try to summarize them here.
First, the CPU time is longer for most of <1mb images, since the zlib de/compression works much faster with data all in memory (than data coming in stream)
And even the memory footprint did get worse for some cases (<1mb images mostly), and it could be a bug or something I cannot tell right now. I didn't get anything yet from the alloc_space profiles, which claim the branch performs better than the original.

Attaching the benchmark results below 🙏

i) ~900kb PNG

(original)
BenchmarkPNGEmbedding-12              13          86746710 ns/op        58313659 B/op       4612 allocs/op
(this branch)
BenchmarkPNGEmbedding-12               3         479347188 ns/op        127033541 B/op      4407 allocs/op
(this branch + sync.Pool)
BenchmarkPNGEmbedding-12               3         461659600 ns/op        73372589 B/op       4370 allocs/op

ii) ~2mb PNG

(original)
BenchmarkPNGEmbedding-12             128           9228263 ns/op        39116499 B/op        900 allocs/op
(this branch)
BenchmarkPNGEmbedding-12             136           8186439 ns/op        16689900 B/op        544 allocs/op
(this branch + sync.Pool)
BenchmarkPNGEmbedding-12             368           3261043 ns/op         1216504 B/op        498 allocs/op

@PuKoren
Copy link
Member

PuKoren commented Jan 19, 2021

the gains on 2mb + sync.Pool are impressive

the allocs/op on the 900kb image is quite high, do you know why there is much more allocs than for the 2mb png?

@naggingant
Copy link
Member Author

Yeah that's really weird, maybe there is some difference in the format (PNG can contain many things)
Now I suspect I did something wrong with the benchmarks, or there is a bug...
Let me get back to you later

This hobby work does not prevent the other development 👍

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