1124847 - Track D3D11 shared texture usage in about:memory

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Sketch of what this would look like

Comment 1

[Bas Schouten (:bas.schouten)]

I don't think the patch is quite what you meant it to be :). Are you going to take this bug Jeff?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

The intent is there, it's just not complete.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: d3d11-memory-usage.patch

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8554761 [details] [diff] [review]
d3d11-memory-usage.patch

Review of attachment 8554761 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +361,5 @@
> +
> +class TextureMemoryMeasurer : public IUnknown
> +{
> +public:
> +    TextureMemoryMeasurer(size_t aMemoryUsed)

Can you fix the indentation and such here to be consistent with the file rather than the source you copy-pasted from? ;-)

We may need to make this threadsafe, I'm uncertain whether D3D makes thread safety guarantees.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bas Schouten (:bas.schouten) from comment #4)
> Comment on attachment 8554761 [details] [diff] [review]
> d3d11-memory-usage.patch
> 
> Review of attachment 8554761 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/TextureD3D11.cpp
> @@ +361,5 @@
> > +
> > +class TextureMemoryMeasurer : public IUnknown
> > +{
> > +public:
> > +    TextureMemoryMeasurer(size_t aMemoryUsed)
> 
> Can you fix the indentation and such here to be consistent with the file
> rather than the source you copy-pasted from? ;-)
> 
> We may need to make this threadsafe, I'm uncertain whether D3D makes thread
> safety guarantees.

This should be threadsafe. D3D is documented to only do a single AddRef and Release so as long as it does those separately in time we should be fine.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c324316ea7a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes a'plenty on Win7.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b544c5bb1a8

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=6000378&repo=mozilla-inbound

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out again for the same crashes as last time. Please run this through Try before attempting to land again so you don't waste other devs' time with a closed tree.
https://hg.mozilla.org/integration/mozilla-inbound/rev/13dd7bfcb9b1

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=6024082&repo=mozilla-inbound

Comment 9

[Jeff Muizelaar [:jrmuizel]]

FWIW, I did run it through try but missed the crashing jobs thinking they were intermittent oranges. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01f869e248fb

Comment 10

[Daniel Holbert [:dholbert]]

(Yeah, there's a bug [in buildbot?] where crashes/aborts/hangs during crashtest & reftest runs on Windows are displayed as orange, when they should be red -- that's bug 1121837.)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I'm confused - you saw orange and just assumed "intermittents"? Orange doesn't automatically mean intermittent - it means there's failures that need to be looked at. Maybe they match a known intermittent, maybe they don't. Especially when you see orange on both opt and debug crashtests and reftests? That should be setting off alarms no matter what color the job is showing.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #11)
> I'm confused - you saw orange and just assumed "intermittents"? Orange
> doesn't automatically mean intermittent - it means there's failures that
> need to be looked at. Maybe they match a known intermittent, maybe they
> don't. Especially when you see orange on both opt and debug crashtests and
> reftests? That should be setting off alarms no matter what color the job is
> showing.

I assumed that the change from nearly all the tests failing to most of them passing meant that the problem was fixed and that the remaining problems were intermittent oranges. It was a bad assumption.

Comment 13

[Saikat Dutta]

What does intermittent orange mean? Can someone elaborate?

Comment 14

[Daniel Holbert [:dholbert]]

We have lots of automated tests, and some small fraction of them fail some small fraction of the time; so test runs will often have one or two of these "intermittent" failures that aren't related to your patch. (We do our best to squash the worst intermittent failures.) Jeff initially assumed the failures on his TryServer run were in this "intermittent" category,  but they were actually real test-failures (and crashes) caused by his patch.

(If you have further questions about this, ask around in irc.mozilla.org #developers, rather than continuing the discussion here -- bug pages aren't really the right place for back-and-forth on general mozilla background-information [with the exception of "good first bugs" & explicitly mentored bugs])

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66f0f0b2bfd2

Comment 16

[Ryan VanderMeulen [:RyanVM]]

(In reply to Carsten Book [:Tomcat] from comment #15)
> https://hg.mozilla.org/mozilla-central/rev/66f0f0b2bfd2

Please avoid including Try syntax in your pushes in the future. Funny thing is that I thought we had a commit hook to prevent that from happening.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #16)
> Please avoid including Try syntax in your pushes in the future. Funny thing
> is that I thought we had a commit hook to prevent that from happening.

Yeah, I've filed bug 1127174 about that issue.

Comment 18

[Nicholas Nethercote [inactive]]

> /* This class get's it's lifetime tied to a D3D texture

Two unnecessary apostrophes in two words. Impressive! :P

Comment 19

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nicholas Nethercote [:njn] from comment #18)
> > /* This class get's it's lifetime tied to a D3D texture
> 
> Two unnecessary apostrophes in two words. Impressive! :P

https://hg.mozilla.org/integration/mozilla-inbound/rev/4be1e72d619b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4be1e72d619b

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

https://hg.mozilla.org/projects/holly/rev/a501183528f3

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8554761 [details] [diff] [review]
d3d11-memory-usage.patch

Approval Request Comment
[User impact if declined]: We'll get less memory usage data
[Describe test coverage new/current, TreeHerder]: Has been on m-c for a little while without any problems.
[Risks and why]: Very low risk. This just adds a memory reporter for d3d11 shared texture usage. It should have no behavioral change.

Comment 23

[Andrew McCreight [:mccr8]]

Also, this may help the investigation of severe memory leaks seen on certain hardware when playing YouTube videos with MSE.

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8554761 [details] [diff] [review]
d3d11-memory-usage.patch

I'm happy to push changes that give us better gfx related data. Aurora+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ac38c94542f