907055 - Have an preference value to enable Compositor render-insomnia

Status: RESOLVED DUPLICATE of bug 956263

(Core :: Graphics, defect)

Description

[u459114]

It's a usefule feature while enabling a new platform.

To figure out rendering performance bottleneck, we need to know where is the bottleneck
1. At content process - image rasterlization/ restyle or other layout activity 
2. At compositor in B2G - GPU composition/ HWC composition.

By this prefernece value, we force GLComposor keep running without taking a rest even if there is no layout transaction from content process. If we find the FPS is still low in this scenario(content process has no rendering action/ GLComposor take all GPU resource), we have more confidence that this platform has GPU performance problem.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] force trigger b2g composition

use androd setprop to trigger force update.
adb shell "setprop debug.compositor.force.update 1"

Comment 2

[Peter Chang[:pchang]]

(In reply to Jerry Shih[:jerry] from comment #1)
> Created attachment 794542 [details] [diff] [review]
> [WIP] force trigger b2g composition
> 
> use androd setprop to trigger force update.
> adb shell "setprop debug.compositor.force.update 1"

Please consider 
a. how to stop force composition 
b. add your code under MOZ_WIDGET_GONK

BTW, are we able to discard the new layer transactions and just re-composition the cached layer transactions? If we stop the force composition, we just re-enable layer transaction.

Comment 3

[Andreas Gal :gal]

If you use a regular gecko pref for this then we could enable/disable this the way we enable/disable the FPS counter and add a button in Gaia for this.

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: part1: read debug setting from fifo

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: part2: force trigger b2g composition

usage:
adb shell
echo "debug.force_update 1" > /data/local/tmp/gfx_debug (enable force update)
echo "debug.force_update 0" > /data/local/tmp/gfx_debug (disable force update)

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to peter chang[:pchang] from comment #2)
> (In reply to Jerry Shih[:jerry] from comment #1)
> 
> Please consider 
> a. how to stop force composition

We can start/stop force composition by sending the cmd to fifo now.

> b. add your code under MOZ_WIDGET_GONK

This mechanism occurs only if defined(MOZ_WIDGET_GONK) && defined(COMPOSITOR_PERFORMANCE_DEBUG) in gfx/layers/CompositorDebug.h

> BTW, are we able to discard the new layer transactions and just
> re-composition the cached layer transactions? If we stop the force
> composition, we just re-enable layer transaction.

No, I just put the ScheduleComposition() into message loop.
To do this, we need to perform deep copy for whole layer tree(including image, graphic buffer, etc.)

Comment 7

[u459114]

Comment on attachment 808452 [details] [diff] [review]
part1: read debug setting from fifo

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +94,5 @@
>  // existing thread.
>  static PlatformThreadId sCompositorThreadID = 0;
>  static MessageLoop* sCompositorLoop = nullptr;
>  
> +//compositor debug prop watcher

// Compositor

@@ +102,5 @@
> +
> +static RefPtr<CompositorDebugPropWatcher> sCompositorDebugPropWatcher;
> +static Mutex sCompositorDebugWatcherMutex("CompositorDebugPropWatcher mutex");
> +
> +class CompositorDebugPropWatcher : public MessageLoopForIO::Watcher, public RefCounted<CompositorDebugPropWatcher>

Comment here to describe the role of this class. 

Why does it need to be inherit from RefCounted?

@@ +105,5 @@
> +
> +class CompositorDebugPropWatcher : public MessageLoopForIO::Watcher, public RefCounted<CompositorDebugPropWatcher>
> +{
> +public:
> +  static CompositorDebugPropWatcher *GetSingleton(void);

static CompositorDebugPropWatcher *Instance();

@@ +107,5 @@
> +{
> +public:
> +  static CompositorDebugPropWatcher *GetSingleton(void);
> +
> +  static bool IsInCompositorDebugPropWatcherThread(void);

You don't need this static function.
You need only check thread id in debug build by the following marcro
MOZ_ASSERT(XRE_GetIOMessageLoop() == MessageLoopForIO::current())

@@ +109,5 @@
> +  static CompositorDebugPropWatcher *GetSingleton(void);
> +
> +  static bool IsInCompositorDebugPropWatcherThread(void);
> +
> +  CompositorDebugPropWatcher();

Make it private

@@ +114,5 @@
> +  virtual ~CompositorDebugPropWatcher();
> +
> +  //Called when you can read() from the fd without blocking.  Note that this
> +  //function is also called when you're at eof (read() returns 0 in this case).
> +  void OnFileCanReadWithoutBlocking(int aFd);

MOZ_OVERRIDE

@@ +165,5 @@
>    sCompositorThreadRefCount = 1;
> +
> +#if defined(MOZ_WIDGET_GONK) && defined(COMPOSITOR_PERFORMANCE_DEBUG)
> +  XRE_GetIOMessageLoop()->PostTask(FROM_HERE,NewRunnableMethod(CompositorDebugPropWatcher::GetSingleton(),&CompositorDebugPropWatcher::StartWatching));
> +#endif

Post this task in CompositorDebugPropWatcher::StartWatch and move origin code in StartWatch to CompositorDebugPropWather::StartInternal, which is a private member function.

Don't reveal internal threading modal to client

@@ +1086,5 @@
> +}
> +
> +CompositorDebugPropWatcher::~CompositorDebugPropWatcher()
> +{
> +

Call StopWatching here?

@@ +1251,5 @@
> +    // with NONBLOCK and then fcntl that away.
> +    fd=open(path.get(),O_RDONLY|O_NONBLOCK);
> +  } while(fd==-1 && errno==EINTR);
> +
> +  if(fd==-1) {

what happen if fd == NULL ?

Comment 8

[u459114]

Comment on attachment 808452 [details] [diff] [review]
part1: read debug setting from fifo

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +115,5 @@
> +
> +  //Called when you can read() from the fd without blocking.  Note that this
> +  //function is also called when you're at eof (read() returns 0 in this case).
> +  void OnFileCanReadWithoutBlocking(int aFd);
> +  void OnFileCanWriteWithoutBlocking(int aFd);

Make OnFileCanReadWithoutBlocking and OnFileCanWriteWithoutBlocking be private.

Suppose you need only export these function
Instance()
Start()
Stop()
Register(const nsString &perfName, valueChagenNotification);

@@ +1153,5 @@
> +  // Trimming whitespace is important because if you do
> +  //   |echo "foo" >> debug_info_trigger|,
> +  // it'll actually write "foo\n" to the fifo.
> +  inputStr.Trim("\b\t\r\n");
> +

Should we prefix a pattern string so that we are sure this string is for CompositorDebugPerfWatcher?

@@ +1173,5 @@
> +    Preferences::SetBool("debug.force_hwc",true);
> +  }
> +  else if(inputStr==NS_LITERAL_CSTRING("debug.force_hwc 0")){
> +    Preferences::SetBool("debug.force_hwc",false);
> +  }

Instead if list all property here, export a register function for clients to tell CompositorDebugPropWatcher which perf value they interest.

Comment 9

[u459114]

I think feedback is more correct then needinfo in this case

Comment 10

[Peter Chang[:pchang]]

Comment on attachment 808452 [details] [diff] [review]
part1: read debug setting from fifo

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +48,5 @@
>  #include "mozilla/layers/CompositorD3D9.h"
>  #endif
>  #include "GeckoProfiler.h"
>  
> +#include "CompositorDebug.h"  //compositor debug option

move compositorDebug.h under #if defined(MOZ_WIDGET_GONK) && defined(COMPOSITOR_PERFORMANCE_DEBUG)

@@ +54,5 @@
> +#include "mozilla/Preferences.h"
> +#include "mozilla/Mutex.h"
> +#include "nsDirectoryServiceDefs.h"
> +
> +#ifdef XP_LINUX

Do you need to support XP or Linux?

@@ +79,5 @@
>  static LayerTreeMap sIndirectLayerTrees;
>  
> +typedef map<uint64_t,CompositorParent*> CompositorMap;
> +static CompositorMap* sCompositorMap;
> +

add #if defined(MOZ_WIDGET_GONK) && defined(COMPOSITOR_PERFORMANCE_DEBUG)

@@ +122,5 @@
> +  void StopWatching(void);
> +
> +private:
> +  int OpenFD(void);
> +

FD is a generic name, please rename to proper name.

@@ +1091,5 @@
> +}
> +
> +bool CompositorDebugPropWatcher::IsInCompositorDebugPropWatcherThread(void)
> +{
> +  if(XRE_GetIOMessageLoop() == MessageLoopForIO::current()){

add one space before '{' and also need to change other parts.

@@ +1101,5 @@
> +CompositorDebugPropWatcher* CompositorDebugPropWatcher::GetSingleton(void)
> +{
> +  {
> +    MutexAutoUnlock watcher_mutex(sCompositorDebugWatcherMutex);
> +

Why do we need this mutex here? Does you try to prevent multiple threads call GetSingleton?

@@ +1213,5 @@
> +  }
> +}
> +
> +int CompositorDebugPropWatcher::OpenFD(void)
> +{

I would suggest to create common utility for fd open/read/write.
And include the utility here for CompositorDebugPropWatcher.

Comment 11

[Benoit Girard (:BenWa)]

Sorry to butt in but can we move as much code as possible outside of CompositorParent in into it's own file? CompositorParent keeps growing and we've already had to split code out of it previously.

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Benoit Girard (:BenWa) from comment #11)
> Sorry to butt in but can we move as much code as possible outside of
> CompositorParent in into it's own file? CompositorParent keeps growing and
> we've already had to split code out of it previously.

The CompositorDebugPropWatcher would place in a separated file but the class CompositorForceUpdateSetter won't.
It is only used in CompositorParent.

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

With "user_pref("layers.offmainthreadcomposition.frame-rate", 0)", it will composite all the time, even if nothing changed.