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

Crash when allocating unreasonable amounts of texture memory #3210

Closed
wants to merge 2 commits into from

Conversation

@nical
Copy link
Collaborator

nical commented Oct 17, 2018

Track texture allocations and crash if we go beyond a certain budget. The budget (2Gb by default) is configurable in the renderer options. I'll experiment with various thresholds in gecko (which probably need to take into account the window size) when this hits mozilla-central.

Fixes #3207.


This change is Reviewable

@nical nical force-pushed the nical:tex-budget branch from 8052882 to ee4857f Oct 17, 2018
@gw3583
Copy link
Collaborator

gw3583 commented Oct 17, 2018

Looks like some of the PF code needs updating:

error[E0308]: mismatched types
  --> webrender\src\gpu_glyph_renderer.rs:66:41
   |
66 |         device.upload_texture_immediate(&area_lut_texture, area_lut_pixels);
   |                                         ^^^^^^^^^^^^^^^^^ expected struct `device::gl::Texture`, found enum `std::result::Result`
   |
   = note: expected type `&device::gl::Texture`
              found type `&std::result::Result<device::gl::Texture, device::gl::AllocError>`
error[E0308]: mismatched types
  --> webrender\src\gpu_glyph_renderer.rs:90:13
   |
90 |             area_lut_texture,
   |             ^^^^^^^^^^^^^^^^ expected struct `device::gl::Texture`, found enum `std::result::Result`
   |
   = note: expected type `device::gl::Texture`
              found type `std::result::Result<device::gl::Texture, device::gl::AllocError>`
error[E0308]: mismatched types
   --> webrender\src\gpu_glyph_renderer.rs:129:13
    |
129 |             texture,
    |             ^^^^^^^ expected struct `device::gl::Texture`, found enum `std::result::Result`
    |
    = note: expected type `device::gl::Texture`
               found type `std::result::Result<device::gl::Texture, device::gl::AllocError>`
error[E0308]: mismatched types
   --> webrender\src\gpu_glyph_renderer.rs:191:46
    |
191 |         self.device.upload_texture_immediate(&path_info_texture, &path_info_texels);
    |                                              ^^^^^^^^^^^^^^^^^^ expected struct `device::gl::Texture`, found enum `std::result::Result`
    |
    = note: expected type `&device::gl::Texture`
               found type `&std::result::Result<device::gl::Texture, device::gl::AllocError>`
error[E0308]: mismatched types
   --> webrender\src\gpu_glyph_renderer.rs:229:60
    |
229 |         self.device.bind_texture(TextureSampler::color(1), &path_info_texture);
    |                                                            ^^^^^^^^^^^^^^^^^^ expected struct `device::gl::Texture`, found enum `std::result::Result`
    |
    = note: expected type `&device::gl::Texture`
               found type `&std::result::Result<device::gl::Texture, device::gl::AllocError>`
error[E0308]: mismatched types
   --> webrender\src\gpu_glyph_renderer.rs:234:36
    |
234 |         self.device.delete_texture(path_info_texture);
    |                                    ^^^^^^^^^^^^^^^^^ expected struct `device::gl::Texture`, found enum `std::result::Result`
    |
    = note: expected type `device::gl::Texture`
               found type `std::result::Result<device::gl::Texture, device::gl::AllocError>`
error: aborting due to 6 previous errors
For more information about this error, try `rustc --explain E0308`.

let report = self.report_memory();
let AllocError::OverBudget(budget) = error;
error!(
"Excessive GPU alloc: tex cache:{}, targets:{}, gpu cache:{}, vertex data: {} budget: {}",

This comment has been minimized.

@bholley

bholley Oct 17, 2018

Contributor

I think it'd be preferable to derive(Display) on MemoryReport and use that here, so that any additional fields we add will automatically get picked up here.

@nical nical force-pushed the nical:tex-budget branch 2 times, most recently from ebf59ac to a31bf6c Oct 17, 2018
@nical
Copy link
Collaborator Author

nical commented Oct 19, 2018

I fixed the failing rawtest and added a commit that prints a message and skips the rawtests that rely on a the device pixel ratio being 1.0 when it is not the case to avoid having to comment them out locally.

@gw3583
Copy link
Collaborator

gw3583 commented Oct 21, 2018

Generally looks good to me - should we change the unwrap calls to expect to provide a bit more context on any crashes that do occur?

@gw3583
gw3583 approved these changes Oct 21, 2018
@@ -1583,6 +1609,8 @@ impl Device {
self.release_depth_target(texture.get_dimensions());
}

self.deallocated_gpu_memory(texture.size_in_bytes());

This comment has been minimized.

@gw3583

gw3583 Oct 21, 2018

Collaborator

Are there any other places in the code we re-size / re-init textures that we need to account for the change in allocation size?

This comment has been minimized.

@nical

nical Oct 21, 2018

Author Collaborator

I didn't find any. Our code to resize texture currently destroys and re-creates bigger textures (with possibly a blit from the old texture to the new one.

This comment has been minimized.

@bholley

bholley Oct 22, 2018

Contributor

Yeah, this was a change I had to make for immutable storage. Texture re-initialization is no more.

}
}

fn log_allocation_failure(&self, error: AllocError) {

This comment has been minimized.

@gw3583

gw3583 Oct 21, 2018

Collaborator

It would be neat to have both the current budget and allocated memory available as a profile counter in the debug profiler overlay. This would allow us to get a sense of total texture allocations on pages as we browse, and make sure that the counters are working correctly.

This comment has been minimized.

@nical

nical Oct 21, 2018

Author Collaborator

Since the budget is there to force a crash when things are so wrong that it's best not to keep the browser alive, I don't think it is very interesting to display.
That said I think that displaying some number that we think is reasonable for an average page, or some number beyond which we believe something worth investigating (but not to the point that we'd crash the browser) would be useful, as well as displaying the current allocated texture memory.

@kvark
Copy link
Member

kvark commented Oct 23, 2018

That try push needs another try

@nical
Copy link
Collaborator Author

nical commented Oct 24, 2018

@nical nical force-pushed the nical:tex-budget branch from a31bf6c to 2a4d35a Oct 24, 2018
@gw3583
Copy link
Collaborator

gw3583 commented Nov 4, 2018

@nical Looks like this is ready to merge now, is that right?

@bholley
Copy link
Contributor

bholley commented Nov 4, 2018

Per discussion on IRC late last week, we should rebase it on top of [1] when that lands to avoid having multiple mechanisms for tracking GPU memory usage.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1504115

@bors-servo
Copy link
Contributor

bors-servo commented Nov 5, 2018

The latest upstream changes (presumably #3269) made this pull request unmergeable. Please resolve the merge conflicts.

@gw3583
Copy link
Collaborator

gw3583 commented Nov 20, 2018

Looks like the bug mentioned above has landed - so I guess this needs a rebase on top of that?

@gw3583
Copy link
Collaborator

gw3583 commented Dec 5, 2018

@nical @bholley Do we just need to get this rebased and then it's ready to land?

@bholley
Copy link
Contributor

bholley commented Dec 6, 2018

It just needs to be rebased to use the global atomic byte count rather than tracking it per-device. That should be trivial, then it can land.

@gw3583
Copy link
Collaborator

gw3583 commented Jan 3, 2019

ping @nical - should we get this rebased and landed?

@nical
Copy link
Collaborator Author

nical commented Jan 3, 2019

Sorry for the delay, I need to more or less re-write the PR so I'll close it for now.

@nical nical closed this Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.