Closed Bug 780330 Opened 7 years ago Closed 7 years ago

Only lock the surface in ThebesLayerBuffer when we need to use it

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp -

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(1 file, 1 obsolete file)

The ioctls and associated overhead from galloc-buffer locking is taking ~5% of a profile of dragging the homescreen, even though we almost never repaint anything.  This is unnecessary and easy to fix.

Matt has a WIP patch.
blocking-basecamp: --- → ?
Chris, I nommed the dependencies of the 60fps bug but perhaps shouldn't have done so since making decisions about them is difficult :)  Any thoughts as to whether or not they should individually block?
As I mentioned in the other bug, setting blocking+ for the dependencies doesn't really make sense because we wouldn't hold the release for any of them individually.

The algorithm is going to be fix the known jank in order of best benefit/cost ration until we hit 60fps.  I lean towards them not individually blocking because I think it gives the wrong impression.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> I lean towards them not
> individually blocking because I think it gives the wrong impression.

Okay, thanks.
blocking-basecamp: ? → -
Attached patch WIP (obsolete) — Splinter Review
I started working on this and realized it's actually much harder than it seems on first blush, because ThebesLayerBuffer::BeginPaint() is how we determine whether or not there's work to do in a given PAINT phase, but that function requires the buffer to be mapped.

To really fix this, we'd need some sort of buffer "promise" (gfxASurface) that could map the buffer when it was actually about to be used, instead of checked against nullptr.  But also was nullptr when there's no buffer.

This patch adds the gymnastics to request buffer mapping from within ThebesLayerBuffer, but I didn't try to compile it.
I suspect what's happening here is that these ioctl()s are us taking and releasing genlock locks.  I further suspect that the actual work being done is quite cheap, but going into the kernel creates a preemption point and we get switched out for other runnable tasks.  But we shouldn't give up our time slice for unnecessary work, so should still fix this.
Comment on attachment 658408 [details] [diff] [review]
WIP

This is not the way I want to go.
Attachment #658408 - Attachment is obsolete: true
OK!  This actually ended up being as easy as I hoped it would be.

This gets all the GraphicBuffer::lock/unlock ioctls out of the profile

http://people.mozilla.com/~bgirard/cleopatra/?report=33ac30f2e8076314694e41c22bda478e6c333d90

and wins us 4-5 fps on the homescreen.
Assignee: matt.woodrow → jones.chris.g
Attachment #660252 - Flags: superreview?(roc)
Attachment #660252 - Flags: review?(matt.woodrow)
Attachment #660252 - Flags: review?(matt.woodrow) → review+
Comment on attachment 660252 [details] [diff] [review]
Avoid mapping/unmapping buffers when we don't use them

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

cool
Attachment #660252 - Flags: superreview?(roc) → superreview+
https://hg.mozilla.org/mozilla-central/rev/b3bf5f7f02b9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.