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

Use BufWriter in imageproc #1996

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

Hoverbear
Copy link
Contributor

Problem

While updating Zola from 14.1 to 16.1 (congratulations and great work!) I noticed that image resizing slowed down significantly.

On my website I don't track my static/processed_images in the main branch, instead I keep a working copy on my gh-pages branch which is published. So, when I was bringing up a new machine I pulled the latest Zola (16.1) and started the initial build process which would process the images.

To my surprise, after nearly 20 minutes on a 32 core machine, it wasn't done resizing around 465 images. Usually this process only takes a couple minutes.

I noticed I could open an image during the build process and watch as it was saved. It seemed to be saving pixel by pixel, which would indeed be much slower than a buffered writer as my proposed solution suggests.

Resolution

Here we use the image::DynamicImage type which has the write_to function:

let mut f = File::create(target_path)?;
match self.format {
Format::Png => {
img.write_to(&mut f, ImageOutputFormat::Png)?;
}
Format::Jpeg(q) => {
img.write_to(&mut f, ImageOutputFormat::Jpeg(q))?;
}
Format::WebP(q) => {
let encoder = webp::Encoder::from_image(&img)
.map_err(|_| anyhow!("Unable to load this kind of image with webp"))?;
let memory = match q {
Some(q) => encoder.encode(q as f32),
None => encoder.encode_lossless(),
};
f.write_all(memory.as_bytes())?;
}
}

According to the write_to docs, it assumes a buffered writer, so we should provide one.

How it happened

In Zola 0.14.1 we depended on image 0.23:

image = "0.23"

This version of image does not include the BufWriter notice on write_to.

In Zola 16.1, we depend on image 0.24:

image = "0.24"

This version includes the warning on write_to.

Performance

Prior to these changes, building took over 26 minutes (I got bored and cancelled):

hoverbear.org on  root [!] via ❄️  impure (nix-shell)
❯ rm -rf static/processed_images/

hoverbear.org on  root [!] via ❄️  impure (nix-shell) 
❯ zola build
Building site...
Checking all internal links with anchors.
> Successfully checked 11 internal link(s) with anchors.
-> Creating 95 pages (0 orphan) and 15 sections
^C

hoverbear.org on  root [!] via ❄️  impure (nix-shell) took 26m2s 
❯ 

With these changes it only took 20 seconds:

hoverbear.org on  root [!] took 2s 
❯ rm -rf static/processed_images/

hoverbear.org on  root [!] 
❯ time ./../../getzola/zola/target/release/zola build
Building site...
Checking all internal links with anchors.
> Successfully checked 11 internal link(s) with anchors.
-> Creating 95 pages (0 orphan) and 15 sections
Done in 20.3s.


real    0m20.503s
user    7m54.969s
sys     0m29.248s

hoverbear.org on  root [!] via ❄️  impure (nix-shell) took 20s 
❯

Sanity check

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

  • Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • Have you created/updated the relevant documentation page(s)?

According to
https://docs.rs/image/0.24.3/image/enum.DynamicImage.html#method.write_to,
it assumes a buffered writer, so we should provide one.

Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear
Copy link
Contributor Author

Hoverbear commented Sep 25, 2022

I spent some time looking at the test failure, but it seems to be unrelated?

test can_render_basic_markdown ... FAILED

failures:

---- can_render_basic_markdown stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Differences ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: components/markdown/tests/snapshots/markdown__can_render_basic_markdown.snap
Snapshot: can_render_basic_markdown
Source: components/markdown/tests/markdown.rs:27
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Expression: body
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
    2     2 │ <p>Hello <em>world</em>
    3     3 │ Hello
    4     4 │ world
    5     5 │ Non rendered emoji :smile:
    6       │-<a href="image.jpg">a link</a>
          6 │+<a href="https://www.getzola.org/test/image.jpg">a link</a>
    7     7 │ <img src="https://www.getzola.org/test/image.jpg" alt="alt text" /></p>
    8     8 │ <h1>some html</h1>
────────────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
thread 'can_render_basic_markdown' panicked at 'snapshot assertion for 'can_render_basic_markdown' failed in line 27', /home/ana/.cargo/registry/src/github.com-1ecc6299db9ec823/insta-1.20.0/src/runtime.rs:516:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Please let me know if it's something I should investigate!

@Keats
Copy link
Collaborator

Keats commented Sep 26, 2022

Nice catch! Don't worry about the error, I'll have a look on my side. That's due to my last change

@Keats Keats merged commit fd0e25e into getzola:next Sep 26, 2022
@Hoverbear
Copy link
Contributor Author

Thanks for all the hard work! :)

Keats pushed a commit that referenced this pull request Feb 16, 2023
According to
https://docs.rs/image/0.24.3/image/enum.DynamicImage.html#method.write_to,
it assumes a buffered writer, so we should provide one.

Signed-off-by: Ana Hobden <operator@hoverbear.org>
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