Status

()

defect
P2
normal
RESOLVED INVALID
6 years ago
5 years ago

People

(Reporter: vlin, Assigned: jerry)

Tracking

({perf})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c=uniformity p= s=2014.06.20.t u=])

Attachments

(5 attachments, 10 obsolete attachments)

6.99 MB, text/html
Details
9.76 MB, text/html
Details
40.04 KB, application/pdf
Details
41.96 KB, image/png
Details
4.44 KB, patch
Details | Diff | Splinter Review
No description provided.
Posted patch ProjectButterWIP0306.patch (obsolete) — Splinter Review
Attachment #8386642 - Flags: feedback?(cku)
Here is our simple idea:
1. Instead of using a system timer, we trigger reflesh by vsync event. (This is what  ProjectButterWIP0306.patch did)
2. Then, handle touch event after vsync recieve, and trigger paint/ layout transaction accordingly. 
                     VSYNC        VSYNC
                     |              |
Timeline        --------------------------------
Content Process      |H|P|T         |H|P|T

T= Handle touch event. Include touch event normalization. (Bug 970751)
P= PreShell::Paint
T= Layer Transaction 

is there anyone has already working on this?
By Layer Transaction, do you mean the work in CompositeParent::CompositeToTarget()?

I can see the benefit of running the compositor off of the vsync as its work is directly related, but can you describe why we would want touch events and PresShell::Paint in the same category?

In particular, it seems that PresShell::Paint() (of potentially things not on screen yet) could take longer than our frame budget and we wouldn't want to delay composition of existing content.

Sorry if I'm just confused about the idea here.

Also, if we do want to drive composition like this then bug 980027 and bug 980321 might help?
See Also: → 980027
See Also: → 970751
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P2
Whiteboard: [c=uniformity p= s= u=]
(In reply to Ben Kelly [:bkelly] from comment #3)
> By Layer Transaction, do you mean the work in
> CompositeParent::CompositeToTarget()?

Layer Transaction means "SendUpdate()" or "SendUpdateNoSwap()" IPC to compositor.
CompositeToTarget() will happen to the next Vsync.

                        VSYNC        VSYNC        VSYNC
                        |              |              |
Timeline           -----------------------------------------
Content Process         |H|P|T         |H|P|T
B2G Process      [TEs]                 |C|            |C|

H= Handle touch event. Include touch event normalization.
P= PreShell::Paint
T= Layer Transaction 
C= Composition
[TEs] = Touch Events
Posted file HalVsyncInfo.h (obsolete) —
The newly created file has to be appended to the previous WIP. (gecko/hal)
Comment on attachment 8386642 [details] [diff] [review]
ProjectButterWIP0306.patch

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

Feedback of HwcCompiser2D.cpp/h

::: widget/gonk/HwcComposer2D.cpp
@@ +121,5 @@
>          mRBSwapSupport = supported ? true : false;
>      }
> +
> +    LOGI("%s %p", __func__, mHwc->registerProcs);
> +    if (mHwc->registerProcs) {

You can bypass mHwc to mCBContext, and handle all the things in that class

Change class definition of HwcComposer2D::cb_context to
class VSyncHandler {
public:
  VSyncHandler() { /*Initiate data member here*/}
  void Init(HwcDevice *hwc);

  static hook_invalidate() ...
  static hook_vsyn() ...
  static hook_hotplug()...

private:
 struct callbacks : public hwc_procs_t {
 ...
 };
 callbacks procs;
};

And, unless VSyncHandler real need to use HwcComposer2d, VSyncHandler should not aware HwcComposer2D at all.

@@ +187,5 @@
> +
> +void
> +HwcComposer2D::vsync(int disp, int64_t timestamp) {
> +    ATRACE_INT("VSYNC", ++mVSyncCount&1);
> +    nsAppShell::NotifyVsyncReceived(timestamp);

Don't bypass vsync signal into nsAppShell. VSyncHandler should dispatch this signal from hwc to hal in this compile unit

::: widget/gonk/HwcComposer2D.h
@@ +63,4 @@
>  
> +    inline void invalidate();
> +    inline void vsync(int disp, int64_t timestamp);
> +    inline void hotplug(int disp, int connected);

Remove these function declaration.
Move all these function into VSyncHandler in .cpp
Attachment #8386642 - Flags: feedback?(cku) → feedback-
Comment on attachment 8386642 [details] [diff] [review]
ProjectButterWIP0306.patch

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

A suggestion. You don't need to do it in WIP patch

::: layout/base/nsRefreshDriver.cpp
@@ +711,5 @@
>        sRegularRateTimer = new PreciseRefreshDriverTimerWindowsDwmVsync(rate, isDefault);
>      }
>  #endif
>      if (!sRegularRateTimer) {
> +      sRegularRateTimer = new HWVsyncRefreshDriverTimer(rate);	 

Have a preference here. By default, use sw timer
Blocks: 970751
Posted patch ProjectButterWIP0312.patch (obsolete) — Splinter Review
Jeremy,

Could you please try this patch ? We don't have equipment to verify the uniformity. I will attach systrace profile later.

The major change is to have a VsyncObserver in gecko/HAL so RefreshDriver and CompositorParent can register to it. The prerequisite is your HWComposer should support vsync callback(Android JB). Or I can design a SW Vsync to support non-JB device.

I also include your patch in Bug 970751. You can see that I ever try to make a VsyncTimer instead for triggering dispatch, but thing gets worse. (I will keep looking into it.)
Attachment #8386642 - Attachment is obsolete: true
Attachment #8387416 - Attachment is obsolete: true
Attachment #8389705 - Flags: feedback?(faraojh)
Posted file mynewtrace.html
Posted patch ProjectButterWIP0313.patch (obsolete) — Splinter Review
WIP~
Touch dispatcher aligns with Vsync as well.
Attachment #8386642 - Attachment is obsolete: true
Attachment #8387416 - Attachment is obsolete: true
Attachment #8389705 - Attachment is obsolete: true
Attachment #8389705 - Flags: feedback?(faraojh)
Attachment #8390391 - Flags: feedback?(faraojh)
Posted patch nsAppShell.patch (obsolete) — Splinter Review
Attachment #8391016 - Flags: feedback?
Thanks Vincent for the patch.

It looks good to me using HwcComposer callback for Vsync. 
I guess it's good idea to have SW Vsync for the such devices which don't support HWcomposer, vsync callback or set the "layers.composer2d.enabled" flag to false.

And I'd like you to consider this patch for nsAppShell.cpp. I only change a way to use vsync observer in GonkInputDispatcher.

Additionally, there is long latency (more than 100ms) when I test drag on a contact list.
I'm trying to figure out why it happens.
Applied your patch to Master(HEAD : 7312341e00e2785419fc4746e291646299ba3018) with the nsAppShell patch I attached here.

The high-speed camera test is in progress. After we get the result, Jeremy will share you it.
Attachment #8390391 - Flags: feedback?(faraojh)
Attachment #8391016 - Flags: feedback? → feedback?(vlin)
(In reply to Jinho Hwang [:jeffhwang] from comment #12)
> Thanks Vincent for the patch.
> 
> It looks good to me using HwcComposer callback for Vsync. 
> I guess it's good idea to have SW Vsync for the such devices which don't
> support HWcomposer, vsync callback or set the "layers.composer2d.enabled"
> flag to false.

Sure. I will make it after we proof the prototype(WIP) is workable.

> 
> And I'd like you to consider this patch for nsAppShell.cpp. I only change a
> way to use vsync observer in GonkInputDispatcher.
> 
> Additionally, there is long latency (more than 100ms) when I test drag on a
> contact list.
> I'm trying to figure out why it happens.
> Applied your patch to Master(HEAD :
> 7312341e00e2785419fc4746e291646299ba3018) with the nsAppShell patch I
> attached here.
> 
> The high-speed camera test is in progress. After we get the result, Jeremy
> will share you it.

Thanks.
If possible, can you have a systrace profile for the test ? I just concerned if the test hits rendering performance issue which is not considered in my current WIP.
Posted file trace_vsync_1.html
Here is one trace file.
I hope it helps your work. 
What I see on the tracefile is that after touch down event is sent, we dispatch more than 5 touch move events but no compositing for those. Possibly, some of those move events are ignored because it didn't exceed the threshold for determine move action.
But, the latency is over 100ms. It's too long.
(In reply to Jinho Hwang [:jeffhwang] from comment #14)
> Created attachment 8391040 [details]
> trace_vsync_1.html
> 
> Here is one trace file.
> I hope it helps your work. 
> What I see on the tracefile is that after touch down event is sent, we
> dispatch more than 5 touch move events but no compositing for those.
> Possibly, some of those move events are ignored because it didn't exceed the
> threshold for determine move action.
> But, the latency is over 100ms. It's too long.

Is there any Content process running at the same time ?
I think Content process got to receive touch event, reflow and then send update to compositor.
The skipped touch move events may be due to the main thread of Content process is busy on flow and painting the layer. This problem dose not surprise me. What I would like to focus on is the uniformity during dragging i.e. the case without rendering or composition bound.
This bug should probably be moved to the Core::Widget:Gonk component considering the code it's touching.
(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:kats@mozilla.com) from comment #16)
> This bug should probably be moved to the Core::Widget:Gonk component
> considering the code it's touching.

The major changes include gonk, hal, layout/RefreshDriver and layer/CompositeParent.
Is it still suitable for Core::Widget:Gonk component.
Posted file WIP.pdf
Introduction to this Bug.
Flags: needinfo?(faraojh)
Comment on attachment 8391016 [details] [diff] [review]
nsAppShell.patch

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

The current WIP is just a prototype. I'm eager to profile the improvement of uniformity now. All the necessary refactorings will be considered once these patches are gonna to be landed.
Attachment #8391016 - Flags: feedback+
Attachment #8391016 - Flags: feedback?(vlin) → feedback+
Results for ProjectButterWIP0313.patch:
- Using ScrollGraph + Orangutan in Contact List
- Tested 5 times
- Graph is deltaY[px] x refresh[#] during drag

The pattern is the same observed using high-speed camera: a good uniformity, but sometimes a peak occurs
Cleaning needinfo, by comment 20
Flags: needinfo?(faraojh)
(In reply to Andre Graziani (:graziani) from comment #20)
> Created attachment 8392701 [details]
> Results_for_ProjectButterWIP0313.png
> 
> Results for ProjectButterWIP0313.patch:
> - Using ScrollGraph + Orangutan in Contact List
> - Tested 5 times
> - Graph is deltaY[px] x refresh[#] during drag
> 
> The pattern is the same observed using high-speed camera: a good uniformity,
> but sometimes a peak occurs

Ok. Great.
I think the peak is probably due to the reflow takes more than 16ms. You can make sure by Systrace.
Bug 931668 is going to improve the performance of re-styling. I'm also looking forward to see if it's helpful.
Vincent, please share your profile data(in a png file) here to explain what you wrote in previous comment
(In reply to C.J. Ku[:CJKu] from comment #23)
> Vincent, please share your profile data(in a png file) here to explain what
> you wrote in previous comment

Here it is.
https://bugzilla.mozilla.org/show_bug.cgi?id=931668#c36
Attachment #8391016 - Attachment is obsolete: true
Posted patch ProjectButterWIP0320.patch (obsolete) — Splinter Review
Move Vsync notification to HwcComposer2D.
Add test code(notifyMotionTest) in nsAppShell.cpp.
Attachment #8390391 - Attachment is obsolete: true
Blocks: Silk
Project butter is awesome. I finally started understanding a lot of what's actually going on. At least for the GeckoInputDispatcher, is there a reason we can't modify the touch device drivers or kernel to also dispatch events at vsyncs rather than having it at the higher 100ms sample rate? Thanks!
Flags: needinfo?(vlin)
No longer blocks: 970751
Component: General → Layout
Depends on: 987529
Flags: needinfo?(vlin)
Product: Firefox OS → Core
(In reply to Mason Chang [:mchang] from comment #26)
> Project butter is awesome. I finally started understanding a lot of what's
> actually going on. At least for the GeckoInputDispatcher, is there a reason
> we can't modify the touch device drivers or kernel to also dispatch events
> at vsyncs rather than having it at the higher 100ms sample rate? Thanks!

Good question.
IMHO~
We can't actually define how touch device driver works after all that belongs to partner's implementation. However, do it in device driver is platform-dependent. 

By the way, it's 100Hz(10ms) sample rate, but not 100ms sample rate. Just fix the typo:)
Hmm maybe not for old devices, but what about future devices? Could we make that a requirement so that we don't have to down sample?
(In reply to Mason Chang [:mchang] from comment #28)
> Hmm maybe not for old devices, but what about future devices? Could we make
> that a requirement so that we don't have to down sample?

So far, B2G was built on top of device driver and even HAL for Android(except for the ongoing TV project). According to my experience working on HWComposer with partner, it's not easy to convince partner to modify driver or HAL for B2G. Furthermore, Android also handle resampling stuff in InputTransport module. If you search "Project Butter" in the internet, you may just see some conceptual article, but not find out this detail. You will do if you trace the code on AOSP.
Posted patch bug-980241-fix.patch (obsolete) — Splinter Review
WIP
Attachment #8394015 - Attachment is obsolete: true
Depends on: 987527
Posted patch bug-980241-fix.patch (obsolete) — Splinter Review
WIP
Attachment #8398391 - Attachment is obsolete: true
No longer depends on: 987529
Posted patch bug-980241-fix.patch (obsolete) — Splinter Review
WIP~
Register with info. of observer type.
Attachment #8401136 - Attachment is obsolete: true
Attachment #8405953 - Flags: feedback?(slee)
Attachment #8405953 - Flags: feedback?(cku)
Comment on attachment 8405953 [details] [diff] [review]
bug-980241-fix.patch

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

To integrate with Bug 987529
1. Change RegisterVsyncObserver(this, VSYNC_TYPE_REFRESH) to VsyncDispatcher::RegisterRefreshTimer(this)
2. Change UnregisterVsyncObserver(this, VSYNC_TYPE_REFRESH) to VsyncDispatcher::UnregisterRefreshTimer(this)

::: layout/base/nsRefreshDriver.cpp
@@ +256,5 @@
> +  virtual void StartTimer()
> +  {
> +    if (!mTimerRunning)
> +      RegisterVsyncObserver(this, VSYNC_TYPE_REFRESH);
> +      mTimerRunning = true;

if (!mTimerRunning)
{
  RegisterVsyncObserver(this, VSYNC_TYPE_REFRESH);
  mTimerRunning = true;
}

@@ +628,5 @@
>  nsRefreshDriver::Shutdown()
>  {
>    // clean up our timers
>    delete sRegularRateTimer;
> +  delete sHWVsyncTimer;

if (sHWVsyncTimer)

@@ +708,5 @@
>                                                             DEFAULT_INACTIVE_TIMER_DISABLE_SECONDS * 1000.0);
>      return sThrottledRateTimer;
>    }
>  
> +  if (Preferences::GetBool("hal.hw-vsync", false)) {

Preferences::GetBool("hal.hw-vsync", false)
Preferences::GetBool("xxx.enablesilk", false)

xxx.enablesilk means we trigger refresh driver by VsyncDispatcher(rename to SilkDispatcher?)
"hal.hw-vsync == true" means we use HW vsync from HWComposer to drive VsyncDispatcher.
"hal.hw-vsync == fasle" means we use SW vsync(a real-time timer) to drive VsyncDispatcher.
"hal.hw-vsync" should be respect in Bug 987527. In this bug, we need only care xxx.enablesilk
Attachment #8405953 - Flags: feedback?(cku) → feedback+
BTW, I think this bug is the clearest one among all silk bugs.
Please refurbish it and ask for review after bug 987524 and bug 987527 ready
Posted patch bug-980241-fix.patch (obsolete) — Splinter Review
Include "gfx.silk-enable" preference.
Attachment #8405953 - Attachment is obsolete: true
Attachment #8405953 - Flags: feedback?(slee)
Include "gfx.silk-enable" preference.
Attachment #8418502 - Attachment is obsolete: true
Assignee: vlin → hshih
Since we decided to explicit call nsRefreshDriver::Tick in VsyncDispatcher
1. nsRefreshDriver needs to know VsyncDispatcher: calls VsyncDispatcher::RegistryRefreshDriver
2. VsyncDispatcher needs to know nsRefreshDriver: calls nsRefreshDriver::Tick

We can't separate change of nsRefreshDriver into another bug(mutual relative). Close this one, let's keep trace progress of SiLK implementation by Bug 987529.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
Whiteboard: [c=uniformity p= s= u=] → [c=uniformity p= s=2014.06.20.t u=]
You need to log in before you can comment on or make changes to this bug.