Closed
Bug 1462355
Opened 7 years ago
Closed 7 years ago
Avoid mutex contention between FrameAnimator/Decoders
Categories
(Core :: Graphics: ImageLib, enhancement, P3)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: aosmond, Assigned: aosmond)
References
Details
(Keywords: perf, Whiteboard: [gfx-noted])
Attachments
(11 files, 6 obsolete files)
21.30 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
3.31 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
18.60 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
2.50 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
9.11 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
42.81 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
10.62 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
14.28 KB,
patch
|
aosmond
:
review+
|
Details | Diff | Splinter Review |
6.88 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
4.11 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
11.69 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
Mutex contention was showing up in my profiling of animated images. We do a lot of unnecessary locking and the following should be fixed:
1) imgFrame's animation parameters are effectively write once, but we set them in imgFrame::Finish when we should be doing it in imgFrame::InitForDecoder so we can remove the lock on them entirely.
2) We want a RawAccessFrameRef in FrameAnimator but the way DrawableSurface works is that we can only get DrawableFrameRef from it. That is an extra round of locking for no real reason.
3) RawAccessFrameRef guarantees there is a data pointer available; it should just save it in the object itself, to avoid locking again just to get that pointer. If all of the other parameters are lockfree, then it just boils down to getting the first reference.
4) We ask for a new RawAccessFrameRef a lot more than we need to, such as when getting the timeout for a frame we will just ask for again later. These should be consolidated.
5) On a related papercut note, I've also observed DrawableFrameRef's malloc appearing in submitted profiles from others. While I doubt it is really significant, we can easily remove it by making ScopedMap work properly with Maybe.
6) We hit the SurfaceCache in FrameAnimator::GetCompositedFrame, even when we don't use the result at all, which is most of the time. We should avoid that.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P3
Whiteboard: [gfx-noted]
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8976552 -
Flags: review?(tnikkel)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8976553 -
Flags: review?(tnikkel)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8976554 -
Flags: review?(tnikkel)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8976555 -
Flags: review?(tnikkel)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8976556 -
Flags: review?(tnikkel)
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8976558 -
Flags: review?(tnikkel)
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8976559 -
Flags: review?(tnikkel)
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8976560 -
Flags: review?(tnikkel)
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8976561 -
Flags: review?(tnikkel)
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8976557 [details] [diff] [review]
0006-Bug-1462355-Part-4.-Remove-imgFrame-GetAnimationData.patch, v1
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39b438c1ff2f9dab757fe85c151013139a5b834a
Attachment #8976557 -
Flags: review?(tnikkel)
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Andrew Osmond [:aosmond] from comment #11)
> Comment on attachment 8976557 [details] [diff] [review]
> 0006-Bug-1462355-Part-4.-Remove-imgFrame-GetAnimationData.patch, v1
>
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=39b438c1ff2f9dab757fe85c151013139a5b834a
The QR reftest failures are WebRender specific. I was originally allocating the first frame of an animated image in a SourceSurfaceSharedData buffer, which doesn't need to be uploaded to the compositor process via TextureHost. I changed this (I recall thinking this, but not believing it would matter) because it now decides it is an animated frame if it gets animated parameters, rather than being a frame other than the first. Using the fallback path, TextureHost doesn't support such large images, and thus it doesn't appear in the reftest. I can switch this back with a bit of bit fiddling.
However the investigation into this revealed downscaling can be improved. Instead of allocating the image into a 32k x 100 for a 100x100 display size, we really ought to downscale to 100x100. More generally, we should support downscaling as long as one of the dimensions fits (e.g. if 200x200 was requested, we should downscale to 200x100). Any stretching done by the drawing should not look any worse as a result of the downscaling, we can reduce the amount of work drawing has to do, and we can reduce our memory footprint to boot. I'll take this as part of a new bug.
Assignee | ||
Comment 13•7 years ago
|
||
Fix reftest failure by moving the frame number into AnimationParams. This allows imgFrame to access it easily and make the same decision regarding type of SourceSurface to store the data in (as well as abstracting the notion of frame numbers away from decoders that don't support animations).
Attachment #8976552 -
Attachment is obsolete: true
Attachment #8976552 -
Flags: review?(tnikkel)
Attachment #8976688 -
Flags: review?(tnikkel)
Assignee | ||
Comment 14•7 years ago
|
||
Attachment #8976553 -
Attachment is obsolete: true
Attachment #8976553 -
Flags: review?(tnikkel)
Attachment #8976689 -
Flags: review?(tnikkel)
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8976554 -
Attachment is obsolete: true
Attachment #8976554 -
Flags: review?(tnikkel)
Attachment #8976690 -
Flags: review?(tnikkel)
Comment 16•7 years ago
|
||
Comment on attachment 8976688 [details] [diff] [review]
0001-Bug-1462355-Part-1a.-Make-imgFrame-animation-paramet.patch, v2
Review of attachment 8976688 [details] [diff] [review]:
-----------------------------------------------------------------
::: image/imgFrame.h
@@ +100,5 @@
> SurfaceFormat aFormat)
> {
> + nsIntRect frameRect(0, 0, aSize.width, aSize.height);
> + AnimationParams animParams { frameRect, FrameTimeout::Forever(),
> + /* aFrameNum */ 0, BlendMethod::OVER,
Passing AnimationParams with framenum == 0 means we change the value of the bool we pass AllocateBufferForImage. Before we passed aIsAnimated (== true in this situation), now we pass false because we are pretending this is the first frame.
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #16)
> Comment on attachment 8976688 [details] [diff] [review]
> 0001-Bug-1462355-Part-1a.-Make-imgFrame-animation-paramet.patch, v2
>
> Review of attachment 8976688 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: image/imgFrame.h
> @@ +100,5 @@
> > SurfaceFormat aFormat)
> > {
> > + nsIntRect frameRect(0, 0, aSize.width, aSize.height);
> > + AnimationParams animParams { frameRect, FrameTimeout::Forever(),
> > + /* aFrameNum */ 0, BlendMethod::OVER,
>
> Passing AnimationParams with framenum == 0 means we change the value of the
> bool we pass AllocateBufferForImage. Before we passed aIsAnimated (== true
> in this situation), now we pass false because we are pretending this is the
> first frame.
Right you are; I will change it to 1. It isn't super harmful since InitForAnimator is just used by FrameAnimator for two frames, but it will unnecessarily chew up an extra file handle on Android, and use a shared surface with WebRender that is never used by the compositor process.
Updated•7 years ago
|
Attachment #8976688 -
Flags: review?(tnikkel) → review+
Updated•7 years ago
|
Attachment #8976689 -
Flags: review?(tnikkel) → review+
Updated•7 years ago
|
Attachment #8976690 -
Flags: review?(tnikkel) → review+
Comment 18•7 years ago
|
||
Comment on attachment 8976555 [details] [diff] [review]
0004-Bug-1462355-Part-2.-Expose-imgFrame-s-data-pointers-.patch, v1
Review of attachment 8976555 [details] [diff] [review]:
-----------------------------------------------------------------
::: image/imgFrame.h
@@ +128,5 @@
> uint32_t aImageFlags,
> gfx::BackendType aBackend);
>
> DrawableFrameRef DrawableRef();
> + RawAccessFrameRef RawAccessRef(bool aOnlyFinished = false);
Comment what aOnlyFinished means please.
@@ +508,4 @@
> }
>
> + uint8_t* Data() const { return mData; }
> + uint32_t DataLength() const { return mFrame->GetImageDataLength(); }
GetImageDataLength calls GetImageBytesPerRow which asserts we are in the monitor, but what guarantees that we have the monitor here?
Updated•7 years ago
|
Attachment #8976557 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 19•7 years ago
|
||
Incorporate review feedback.
Attachment #8976688 -
Attachment is obsolete: true
Attachment #8976936 -
Flags: review+
Assignee | ||
Comment 20•7 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #18)
> Comment on attachment 8976555 [details] [diff] [review]
> 0004-Bug-1462355-Part-2.-Expose-imgFrame-s-data-pointers-.patch, v1
>
> Review of attachment 8976555 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: image/imgFrame.h
> @@ +128,5 @@
> > uint32_t aImageFlags,
> > gfx::BackendType aBackend);
> >
> > DrawableFrameRef DrawableRef();
> > + RawAccessFrameRef RawAccessRef(bool aOnlyFinished = false);
>
> Comment what aOnlyFinished means please.
>
Done.
> @@ +508,4 @@
> > }
> >
> > + uint8_t* Data() const { return mData; }
> > + uint32_t DataLength() const { return mFrame->GetImageDataLength(); }
>
> GetImageDataLength calls GetImageBytesPerRow which asserts we are in the
> monitor, but what guarantees that we have the monitor here?
Hm correct. I removed DataLength() as it wasn't used. My brain thought it made sense in a completionist sense.
Attachment #8976938 -
Flags: review?(tnikkel)
Updated•7 years ago
|
Attachment #8976555 -
Attachment is obsolete: true
Attachment #8976555 -
Flags: review?(tnikkel)
Assignee | ||
Comment 21•7 years ago
|
||
Attachment #8980258 -
Flags: review?(tnikkel)
Updated•7 years ago
|
Attachment #8976556 -
Flags: review?(tnikkel) → review+
Updated•7 years ago
|
Attachment #8976938 -
Flags: review?(tnikkel) → review+
Updated•7 years ago
|
Attachment #8976558 -
Flags: review?(tnikkel) → review+
Comment 22•7 years ago
|
||
Comment on attachment 8976559 [details] [diff] [review]
0008-Bug-1462355-Part-6.-Reuse-RawAccessFrameRef-in-Frame.patch, v1
Review of attachment 8976559 [details] [diff] [review]:
-----------------------------------------------------------------
::: image/FrameAnimator.h
@@ +347,5 @@
> * advanced, and its resulting dirty rect.
> */
> RefreshResult AdvanceFrame(AnimationState& aState,
> DrawableSurface& aFrames,
> + RawAccessFrameRef& aCurrentFrame,
Comment the in/out param behaviour of this please.
Assignee | ||
Comment 23•7 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #22)
> Comment on attachment 8976559 [details] [diff] [review]
> 0008-Bug-1462355-Part-6.-Reuse-RawAccessFrameRef-in-Frame.patch, v1
>
> Review of attachment 8976559 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: image/FrameAnimator.h
> @@ +347,5 @@
> > * advanced, and its resulting dirty rect.
> > */
> > RefreshResult AdvanceFrame(AnimationState& aState,
> > DrawableSurface& aFrames,
> > + RawAccessFrameRef& aCurrentFrame,
>
> Comment the in/out param behaviour of this please.
Done.
Attachment #8976559 -
Attachment is obsolete: true
Attachment #8976559 -
Flags: review?(tnikkel)
Attachment #8980679 -
Flags: review?(tnikkel)
Updated•7 years ago
|
Attachment #8980679 -
Flags: review?(tnikkel) → review+
Updated•7 years ago
|
Attachment #8976560 -
Flags: review?(tnikkel) → review+
Updated•7 years ago
|
Attachment #8976561 -
Flags: review?(tnikkel) → review+
Updated•7 years ago
|
Attachment #8980258 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 24•7 years ago
|
||
Lots of parts here, so let's try not to need to back this out ;).
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e41490f947ec1e4bb98e08ff72b8b7fe18262c2d
Assignee | ||
Comment 25•7 years ago
|
||
(In reply to Andrew Osmond [:aosmond] from comment #24)
> Lots of parts here, so let's try not to need to back this out ;).
>
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e41490f947ec1e4bb98e08ff72b8b7fe18262c2d
Ugh. Speaking of failures. Maybe cannot be copied if it is in a function parameter? Ugh. This may be a tricky one to fix.
Assignee | ||
Comment 26•7 years ago
|
||
Fixed this by passing the filter config structs by const reference. Seems to have done the trick, in addition to saving some unnecessary copies on the stack :).
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccf9f30b0cf2272df1db7c0ec61a69ce7d2e2700
Comment 27•7 years ago
|
||
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/259ee94de92c
Part 1a. Make imgFrame animation parameters threadsafe. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a0a6e103d8
Part 1b. Update Decoder and SurfacePipe plumbing to use updated imgFrame methods. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8ec97c5f5f
Part 1c. Make individual image decoders to use updated Decoder/SurfacePipe methods. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea8add8bacc
Part 2. Expose imgFrame's data pointers via RawAccessFrameRef. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/45406c2b9e9c
Part 3. Make FrameAnimator use the new imgFrame/RawAccessFrameRef methods. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/93bdeed04a6b
Part 4. Remove imgFrame::GetAnimationData as it is no longer used. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/c67a6f1315b4
Part 5. Avoid converting from DrawableFrameRef to RawAccessFrameRef. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7331d229cc8
Part 6. Reuse RawAccessFrameRef in FrameAnimator where possible. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/baeab7da1768
Part 7. Don't hit the SurfaceCache in FrameAnimator::GetCompositedFrame if possible. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d412f560489
Part 8. Avoid allocating on the heap in DrawableFrameRef. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b516954e103
Part 9. Lock animated imgFrame objects at creation rather than deferring. r=tnikkel
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/259ee94de92c
https://hg.mozilla.org/mozilla-central/rev/87a0a6e103d8
https://hg.mozilla.org/mozilla-central/rev/eb8ec97c5f5f
https://hg.mozilla.org/mozilla-central/rev/3ea8add8bacc
https://hg.mozilla.org/mozilla-central/rev/45406c2b9e9c
https://hg.mozilla.org/mozilla-central/rev/93bdeed04a6b
https://hg.mozilla.org/mozilla-central/rev/c67a6f1315b4
https://hg.mozilla.org/mozilla-central/rev/a7331d229cc8
https://hg.mozilla.org/mozilla-central/rev/baeab7da1768
https://hg.mozilla.org/mozilla-central/rev/8d412f560489
https://hg.mozilla.org/mozilla-central/rev/9b516954e103
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•