751764 - Put endDrawing instrumentation back in fennec

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[cmtalbert]

Attachment: a braindead attempt

So, Mark and I finally decided that the issue with endDrawing had to be a Fennec problem. We had been thinking the issues there were related to our testing tool. But, after finally exhausting all our options inside our test tool, we started looking at Fennec.  It looks like when mobile/base/gfx/GeckoSoftwareLayerClient.java was deprecated in favor of mobile/base/gfx/GeckoLayerClient.java the endDrawing stuff was carried over, and then later removed from there when the endDrawing method was removed from that file.

After looking around the code a little, I found what appears to be a possible place to put this back into the code, so I have a patch to attach. Testing it, it doesn't seem to render any content, so it could be that my LOG statement is starving our rendering threads (so this may *not* be the right place to put it).

Any advice you have is welcome.

If you want to try the build it's here: http://people.mozilla.org/~ctalbert/mobile_perf/fennec-15.0a1.en-US.android-arm.apk

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Drive-by comment: Logging on the compositor thread generally do have a noticeable impact on framerate, although it shouldn't be starving anything. endDrawing was taken out because the JNI overhead of the call was also impacting compositor performance.

Comment 2

[cmtalbert]

(In reply to Kartikaya Gupta (:kats) from comment #1)
> Drive-by comment: Logging on the compositor thread generally do have a
> noticeable impact on framerate, although it shouldn't be starving anything.
> endDrawing was taken out because the JNI overhead of the call was also
> impacting compositor performance.

So, then should I not be trying to add it back?
-->WONTFIX?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I would say yes, you shouldn't be trying to add it back. But I lack context of what you're trying to do in the first place. Do you need it for some measurement/test?

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 620892 [details] [diff] [review]
a braindead attempt

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

Chris would know better than me if this is correct or not

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 620892 [details] [diff] [review]
a braindead attempt

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

I don't know why this would stop rendering, it looks benign enough... I'm also afraid this code has changed so much that I don't fully understand what's happening in LayerRenderer anymore. I'll get back to this once I have a better understanding of what's going on.

Comment 6

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #5)
> Comment on attachment 620892 [details] [diff] [review]
> a braindead attempt
> 
> Review of attachment 620892 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know why this would stop rendering, it looks benign enough... I'm
> also afraid this code has changed so much that I don't fully understand
> what's happening in LayerRenderer anymore. I'll get back to this once I have
> a better understanding of what's going on.

I still don't know why this causes deadlock, but if you're looking to time how long Gecko takes to render, this won't do that - this will approximately time how long it takes to render a frame in the compositor. Is this what you wanted?

I think begin/endDrawing in the old code used to be called when Gecko was drawing content.

Comment 7

[cmtalbert]

(In reply to Chris Lord [:cwiiis] from comment #6)
> (In reply to Chris Lord [:cwiiis] from comment #5)
> > Comment on attachment 620892 [details] [diff] [review]
> > a braindead attempt
> > 
> > Review of attachment 620892 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't know why this would stop rendering, it looks benign enough... I'm
> > also afraid this code has changed so much that I don't fully understand
> > what's happening in LayerRenderer anymore. I'll get back to this once I have
> > a better understanding of what's going on.
> 
> I still don't know why this causes deadlock, but if you're looking to time
> how long Gecko takes to render, this won't do that - this will approximately
> time how long it takes to render a frame in the compositor. Is this what you
> wanted?
> 
> I think begin/endDrawing in the old code used to be called when Gecko was
> drawing content.

I'd rather time the drawing of content. That's what I was trying to do. Where should I look for that?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

For that you probably want to wrap something like nsWindow::OnDraw. That includes draw time and the transaction to the compositor.

Comment 9

[Chris Lord [:cwiiis]]

Comment on attachment 620892 [details] [diff] [review]
a braindead attempt

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

f- to get out of my queue, it's been established that this doesn't do what you were intending.

Comment 10

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.