Closed Bug 1124847 Opened 5 years ago Closed 5 years ago

Track D3D11 shared texture usage in about:memory

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink])

Attachments

(1 file, 1 obsolete file)

No description provided.
Blocks: 1123465
I don't think the patch is quite what you meant it to be :). Are you going to take this bug Jeff?
Flags: needinfo?(jmuizelaar)
The intent is there, it's just not complete.
Assignee: nobody → jmuizelaar
Flags: needinfo?(jmuizelaar)
Attachment #8553301 - Attachment is obsolete: true
Attachment #8554761 - Flags: review?(bas)
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.
Attachment #8554761 - Flags: review?(bas) → review+
(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.
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
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
(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.)
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/66f0f0b2bfd2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(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.
Flags: needinfo?(jmuizelaar)
(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.
Flags: needinfo?(jmuizelaar)
> /* This class get's it's lifetime tied to a D3D texture

Two unnecessary apostrophes in two words. Impressive! :P
(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 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.
Attachment #8554761 - Flags: approval-mozilla-aurora?
Also, this may help the investigation of severe memory leaks seen on certain hardware when playing YouTube videos with MSE.
Whiteboard: [MemShrink]
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+
Attachment #8554761 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.