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

Random memory leak + crash with SVGs #841

Closed
Jammyjamjamman opened this issue Apr 21, 2021 · 8 comments
Closed

Random memory leak + crash with SVGs #841

Jammyjamjamman opened this issue Apr 21, 2021 · 8 comments
Labels
bug Something isn't working
Milestone

Comments

@Jammyjamjamman
Copy link

Jammyjamjamman commented Apr 21, 2021

Iced 0.3.0 sometimes freezes + memory leaks on Linux when rendering a Container with SVGs + other widgets.

From debugging. I narrowed down the bug to an infinite loop happening here: https://github.com/RazrFalcon/tiny-skia/blob/2e6aa247308867f2715399d12dc2afb454b925c7/src/dash.rs#L429-L446 .

Here's steps which will reproduce it on a project I'm working on (Caution: may crash your computer):

  1. Clone https://github.com/TelluricDeckay/telluricdeckay TelluricDeckay/telluricdeckay@4d56dcc (Apologies for my horrible code xD.)
  2. Inside the repo, copy config_h.rs.in' to config_h.rs.
  3. cargo run
  4. Select "start" -> "5 player"
  5. If the window freezes, you've reproduced the bug. If the page loads, close the program and repeat steps 3 and 4 until it does freeze. (Usually freezes after up to 5 goes. But it can take more than that.)

Removing SVGs stops the freezes.

@andy5995
Copy link

andy5995 commented Apr 21, 2021

I'm the co-maintainer of the project @Jammyjamjamman mentioned above (Telluric Deckay).

  • OS: Debian Buster (Jammy's using Manjaro)
  • Rust: v1.51.0

The last time I ran it, all my RAM was used, then my swap file. My computer slowed to a crawl becoming unusable (unable to kill the app or switch to a terminal). I was running the app through gdb and waited for the the app to crash.

image

I was able to kill the app at that point.

Me and Jammy are both on the iced Zulip server, so if you'd like to discuss this a little there for more info, just give us a ping.

@andy5995
Copy link

Here's steps which will reproduce it on a project I'm working on (Caution: may crash your computer):

1. Clone https://github.com/TelluricDeckay/telluricdeckay (Apologies for my horrible code xD.)

The trunk (master) branch now uses pngs not svgs, so anyone wanting to try to reproduce should clone using the commit before the change

TelluricDeckay/telluricdeckay@4d56dcc

@hecrj
Copy link
Member

hecrj commented May 4, 2021

I think this may be related to #786 and #820.

@hecrj hecrj added the bug Something isn't working label May 4, 2021
@hecrj hecrj added this to the 0.4.0 milestone May 4, 2021
@aentity
Copy link
Contributor

aentity commented May 30, 2021

I have also encountered this, and after much investigation, I think I have found source (but do not understand exact cause).

It looks like in some cases, the dimensions return by svg rasterizer is not same as the rounded dimensions iced sends into wgpu/image.rs, for example, after patch like this:

diff --git a/wgpu/src/image.rs b/wgpu/src/image.rs
index 5511565..47869dd 100644
--- a/wgpu/src/image.rs
+++ b/wgpu/src/image.rs
@@ -305,6 +305,7 @@ impl Pipeline {
         target: &wgpu::TextureView,
         _scale: f32,
     ) {
+        println!("=== Drawing {} images", images.len());
         let instances: &mut Vec<Instance> = &mut Vec::new();
 
         #[cfg(feature = "image_rs")]
@@ -337,7 +338,6 @@ impl Pipeline {
                 #[cfg(feature = "svg")]
                 layer::Image::Vector { handle, bounds } => {
                     let size = [bounds.width, bounds.height];
-
                     if let Some(atlas_entry) = vector_cache.upload(
                         handle,
                         size,
@@ -364,8 +364,10 @@ impl Pipeline {
         }
 
         let texture_version = self.texture_atlas.layer_count();
+        println!("texture cap {}", self.texture_atlas.layers.capacity());
 
         if self.texture_version != texture_version {
+            println!("Texture atlas has grown: {}", texture_version);
             log::info!("Atlas has grown. Recreating bind group...");
 
             self.texture =
@@ -404,7 +406,6 @@ impl Pipeline {
         while i < total {
             let end = (i + Instance::MAX).min(total);
             let amount = end - i;
-
             let mut instances_buffer = staging_belt.write_buffer(
                 encoder,
                 &self.instances,
@@ -461,6 +462,7 @@ impl Pipeline {
 
             i += Instance::MAX;
         }
+        println!("=== Done");
     }
 
     pub fn trim_cache(&mut self) {
diff --git a/wgpu/src/image/atlas.rs b/wgpu/src/image/atlas.rs
index 660ebe4..3042dbd 100644
--- a/wgpu/src/image/atlas.rs
+++ b/wgpu/src/image/atlas.rs
@@ -16,7 +16,7 @@ pub const SIZE: u32 = 2048;
 pub struct Atlas {
     texture: wgpu::Texture,
     texture_view: wgpu::TextureView,
-    layers: Vec<Layer>,
+    pub layers: Vec<Layer>,
 }
 
 impl Atlas {
diff --git a/wgpu/src/image/vector.rs b/wgpu/src/image/vector.rs
index ab0f67d..7579a19 100644
--- a/wgpu/src/image/vector.rs
+++ b/wgpu/src/image/vector.rs
@@ -77,6 +77,7 @@ impl Cache {
             (scale * width).round() as u32,
             (scale * height).round() as u32,
         );
+        println!("  Uploading {} {}x{}", id, width, height);
 
         // TODO: Optimize!
         // We currently rerasterize the SVG when its size changes. This is slow
@@ -126,6 +127,7 @@ impl Cache {
                 })
                 .flatten()
                 .collect();
+                println!("  Allocated {} {}x{}", id, width, height);
 
                 let allocation = texture_atlas.upload(
                     width,

I see the following outputs for some svgs i have :

=== Drawing 3 images
Uploading 8798714200906558251 124x128 to gpu
Creating a new svg with 8798714200906558251 124x128
Allocated to texture atlas: Contiguous(Partial { layer: 0, region: Region { id: AllocId(88), rectangle: Box2D((250, 475), (375, 603)) } })
Inserting rasterized at 8798714200906558251 125x128
Uploading 18292754920582805357 128x128 to gpu
Uploading 10023106295405620567 128x103 to gpu
texture cap 1
=== Done
=== Drawing 3 images
Uploading 8798714200906558251 124x128 to gpu
Creating a new svg with 8798714200906558251 124x128
Allocated to texture atlas: Contiguous(Partial { layer: 0, region: Region { id: AllocId(89), rectangle: Box2D((250, 603), (375, 731)) } })
Inserting rasterized at 8798714200906558251 125x128
Uploading 18292754920582805357 128x128 to gpu
Uploading 10023106295405620567 128x103 to gpu
texture cap 1
=== Done

So when we go to render svgs, we check if been cached here:

        if self.rasterized.contains_key(&(id, width, height)) {
            let _ = self.svg_hits.insert(id);
            let _ = self.rasterized_hits.insert((id, width, height));

            return self.rasterized.get(&(id, width, height));
        }

We use incoming width, height to see if cached, and in case of prints above, we have failed.

Insert to cache is done after getting raster image with dimensions return from resvg, we cache it using these image dimensions and insert into rasterized:

                let img = resvg::render(
                    tree,
                    if width > height {
                        usvg::FitTo::Width(width)
                    } else {
                        usvg::FitTo::Height(height)
                    },
                    None,
                )?;
                let width = img.width();
                let height = img.height();

                let mut rgba = img.take().into_iter();

                // TODO: Perform conversion in the GPU
                let bgra: Vec<u8> = std::iter::from_fn(move || {
                    use std::iter::once;

                    let r = rgba.next()?;
                    let g = rgba.next()?;
                    let b = rgba.next()?;
                    let a = rgba.next()?;

                    Some(once(b).chain(once(g)).chain(once(r)).chain(once(a)))
                })
                .flatten()
                .collect();

                let allocation = texture_atlas.upload(
                    width,
                    height,
                    bytemuck::cast_slice(bgra.as_slice()),
                    device,
                    encoder,
                )?;

                let _ = self.svg_hits.insert(id);
                let _ = self.rasterized_hits.insert((id, width, height));
                let _ = self.rasterized.insert((id, width, height), allocation);

We can see that the dimensions are off by 1; iced says they are 124x128, but rasterizer returns 125x128; the iced system caches the 125x128 + id, and system loops.
We come back again, and get 124x128, check our cache, do not have (we cached 125x128), and loop continues with this ever increasing memory usage (in addition to gpu strain from many uploads + gpu memory usage).

I believe this is heart of issue. I tried eduardobraun/wgpu-rs@a62dd38 but it did not fix this issue for me, and after investigating, i can see cache grows unbounded, in this case where dimensions are off by one.

Now, i do not understand why dimensions are off by one; i check and it seems resvg uses dimensions and rounds, similar to iced. I thought perhaps ceil would help (and it does for example, on macos, but then leak is on ios).

I am wondering if perhaps it is related to dpi in these cases not being treated correct.

Anyway, this is issue for me, and very problematic, memory quickly grows out of control, unbounded. I believe it is cause of #873 i see sometimes.

I believe this also explains why issue is not visible on glow backend, because the caching logic is inside wgpu/image/vector.rs upload function.

@aentity
Copy link
Contributor

aentity commented Jun 3, 2021

i have now made repro, if you run on macos and move mouse around (so we request redraw), you may see following:

=== Drawing 3 images
Upload 13483474518901529274 112x128
  Allocating 13483474518901529274 113x128
Upload 6665452810276749970 128x100
Upload 15100124890594133699 128x103
=== Done
=== Drawing 3 images
Upload 13483474518901529274 112x128
  Allocating 13483474518901529274 113x128
Upload 6665452810276749970 128x100
Upload 15100124890594133699 128x103
=== Done
=== Drawing 3 images
Upload 13483474518901529274 112x128
  Allocating 13483474518901529274 113x128
Upload 6665452810276749970 128x100
Upload 15100124890594133699 128x103
=== Done
=== Drawing 3 images
Upload 13483474518901529274 112x128
  Allocating 13483474518901529274 113x128
Upload 6665452810276749970 128x100
Upload 15100124890594133699 128x103
=== Done

it is quite serious issue, i hit consistently on macos and ios, makes application unusable and then crash.

@hecrj can you try the repro i posted?

@aentity
Copy link
Contributor

aentity commented Jun 7, 2021

on the repro branch, it is simple like this:

cargo run -p repro

aentity added a commit to aentity/iced that referenced this issue Jul 14, 2021
Calls ceil() on dimension bounds as this appears fix svg memory
unbounded usage because no longer cache miss.

The height and width return from resvg seem to always be ceiling
of float dimensions, so we try to match.
@aentity
Copy link
Contributor

aentity commented Jul 14, 2021

I have pushed I think fix in the branch. I hit this always on every platform (windows, linux, mac). It makes iced not possible to use, and I feel it is urgent.

I have checked incoming dimensions and if there is any fractional component, we previously rounded; but resvg always appears to return ceiling of dimensions, so I use ceiling. It fixes all my problems.

@hecrj @Jammyjamjamman @andy5995 could you please test this branch? (it works with wgpu 0.8 and winit 0.24, you can rebase it if you use wgpu 0.9 and winit 0.25).

Lastly, I have added a warn! to see when it allocates, to easily test if it fixes for you (you should only see once on creation and when resizing or changing svgs, for example).

Thank you for your time.

aentity added a commit to aentity/iced that referenced this issue Jul 14, 2021
Calls ceil() on dimension bounds as this appears fix svg memory
unbounded usage because no longer cache miss.

The height and width return from resvg seem to always be ceiling
of float dimensions, so we try to match.
hecrj added a commit that referenced this issue Jul 21, 2021
Use ceil on svg dimensions, fix svg memory usage ref #841
@hecrj
Copy link
Member

hecrj commented Jan 20, 2022

Fixed by #952.

@hecrj hecrj closed this as completed Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants