Botond Ballo [:botond] Assignee
	Description • 11 years ago

It would be nice to have an APZC tree dump in the same vein as a layer dump.

Some desirable features of this dump:
  - show all layers
  - for layers that have an APZC, indicate this
      - print out some information about its APZC, such as
          - its ScrollableLayerGuid
          - some metrics
          - the content element for which it was created

	Botond Ballo [:botond] Assignee
	Comment 1 • 11 years ago

I have a patch in my local tree that does this. It uses some utilities which are contained in a separate repository at https://bitbucket.org/botond/mozilla-debug/.

I would like to clean up those utilities and the patch at some point and get them into m-c. Until then, I'm posting the patch as-is in case it's useful to someone.

	Botond Ballo [:botond] Assignee
	Comment 9 • 11 years ago

The attached patches implement the required enhancements to the gfx logging utility. Tomorrow I will post patches that implement the actual APZC tree logging using the enhanced logging utility.

	Bas Schouten (:bas.schouten)
	Comment 10 • 11 years ago

Comment on attachment 8363355 [details] [diff] [review]
Part 5 - Allow writing a log statement that does not end in a newline

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

Do you know whether the compiler will compile this into branchless code? I suspect so but I'd like us to be sure.

	Botond Ballo [:botond] Assignee
	Comment 11 • 11 years ago

(In reply to Bas Schouten (:bas.schouten) from comment #10)
> Comment on attachment 8363355 [details] [diff] [review]
> Part 5 - Allow writing a log statement that does not end in a newline
> 
> Review of attachment 8363355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you know whether the compiler will compile this into branchless code? I
> suspect so but I'd like us to be sure.

Maybe we can add a MOZ_LIKELY around the condition?

	Botond Ballo [:botond] Assignee
	Comment 16 • 11 years ago

All right, that should be all the patches.

BenWa, I'm asking you for feedback on how I conditioned the logging on the pref (see also the Part 7 patch) to make sure it's OK for performance.

	Timothy Nikkel (:tnikkel)
	Comment 17 • 11 years ago

Comment on attachment 8363891 [details] [diff] [review]
Part 9 - Add a content description field to FrameMetrics and populate it if the apz.printtree pref is on

> APZCTreeManager::APZCTreeManager()
>     : mTreeLock("APZCTreeLock"),
>       mTouchCount(0)
> {
>   MOZ_ASSERT(NS_IsMainThread());
>   AsyncPanZoomController::InitializeGlobalState();
>+  Preferences::AddBoolVarCache(&gPrintApzcTree, "apz.printtree", gPrintApzcTree);
> }

We should probably call AddBoolVarCache only once, instead of each time we create a APZCTreeManager.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 11 years ago

Comment on attachment 8363890 [details] [diff] [review]
Part 8 - Add a Describe() method to nsIContent

This should be using an outparam for the string for both methods, not a return value.

>+#include <sstream>

Why is this needed?

Attribute values can contain newlines, so if you're really relying on Describe() to produce one-line strings, you need to change how you're dealing with attributes.  On the other hand, if the "short" and "one-line" bits are aspirational and not actual requirements then this is probably ok.

	Botond Ballo [:botond] Assignee
	Comment 19 • 11 years ago

(In reply to Timothy Nikkel (:tn) from comment #17)
> Comment on attachment 8363891 [details] [diff] [review]
> Part 9 - Add a content description field to FrameMetrics and populate it if
> the apz.printtree pref is on
> 
> > APZCTreeManager::APZCTreeManager()
> >     : mTreeLock("APZCTreeLock"),
> >       mTouchCount(0)
> > {
> >   MOZ_ASSERT(NS_IsMainThread());
> >   AsyncPanZoomController::InitializeGlobalState();
> >+  Preferences::AddBoolVarCache(&gPrintApzcTree, "apz.printtree", gPrintApzcTree);
> > }
> 
> We should probably call AddBoolVarCache only once, instead of each time we
> create a APZCTreeManager.

Yeah, I just discovered this patch has other problems (the variable is only initialized in the parent process on B2G, but RecordFrameMetrics() needs to read it in the child process). I'll have to rethink this.

	Botond Ballo [:botond] Assignee
	Comment 20 • 11 years ago

Comment on attachment 8363891 [details] [diff] [review]
Part 9 - Add a content description field to FrameMetrics and populate it if the apz.printtree pref is on

This patch does not work. I will reupload a correct version shortly.

	Botond Ballo [:botond] Assignee
	Comment 21 • 11 years ago

Revised Part 8 patch to address review comments. The "one line" part is indeed aspirational, as this is method is meant to be used for debugging.

	Botond Ballo [:botond] Assignee
	Comment 22 • 11 years ago

Whoops, forgot to update one part. Fixed.

	Benoit Girard (:BenWa)
	Comment 23 • 11 years ago

Comment on attachment 8363893 [details] [diff] [review]
Part 10 - Print the APZC tree for debugging

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +48,5 @@
>        mTouchCount(0)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    AsyncPanZoomController::InitializeGlobalState();
>    Preferences::AddBoolVarCache(&gPrintApzcTree, "apz.printtree", gPrintApzcTree);

We can't read preference off the main thread. This needs to be done from gfxPlatform. IMO the preference should probably be layers.apzc.dump or under gfx. Ideally pref TLD-ish thing should be modules.

@@ +49,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    AsyncPanZoomController::InitializeGlobalState();
>    Preferences::AddBoolVarCache(&gPrintApzcTree, "apz.printtree", gPrintApzcTree);
> +  sApzcTreeLog.ConditionOnPref(&gPrintApzcTree);

I do like the idea of a preference conditional logging module.

@@ +130,5 @@
>    Collect(mRootApzc, &apzcsToDestroy);
>    mRootApzc = nullptr;
>  
>    if (aRoot) {
> +    sApzcTreeLog << "[start]\n";

I'd rather have this logging resemble the layers.dump more closely. Not a fan of logging [start]/[end], esp. without context, it should be more descriptive.

	Botond Ballo [:botond] Assignee
	Comment 24 • 11 years ago

(In reply to Benoit Girard (:BenWa) from comment #23)
> Comment on attachment 8363893 [details] [diff] [review]
> Part 10 - Print the APZC tree for debugging
> 
> Review of attachment 8363893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/APZCTreeManager.cpp
> @@ +48,5 @@
> >        mTouchCount(0)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >    AsyncPanZoomController::InitializeGlobalState();
> >    Preferences::AddBoolVarCache(&gPrintApzcTree, "apz.printtree", gPrintApzcTree);
> 
> We can't read preference off the main thread. This needs to be done from
> gfxPlatform. IMO the preference should probably be layers.apzc.dump or under
> gfx. Ideally pref TLD-ish thing should be modules.

AsyncPanZoomController::InitializeGlobalState() does the exact same thing to other APZ preferences.

	Botond Ballo [:botond] Assignee
	Comment 25 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #24)
> (In reply to Benoit Girard (:BenWa) from comment #23)
> > Comment on attachment 8363893 [details] [diff] [review]
> > Part 10 - Print the APZC tree for debugging
> > 
> > Review of attachment 8363893 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/composite/APZCTreeManager.cpp
> > @@ +48,5 @@
> > >        mTouchCount(0)
> > >  {
> > >    MOZ_ASSERT(NS_IsMainThread());
> > >    AsyncPanZoomController::InitializeGlobalState();
> > >    Preferences::AddBoolVarCache(&gPrintApzcTree, "apz.printtree", gPrintApzcTree);
> > 
> > We can't read preference off the main thread. This needs to be done from
> > gfxPlatform. IMO the preference should probably be layers.apzc.dump or under
> > gfx. Ideally pref TLD-ish thing should be modules.
> 
> AsyncPanZoomController::InitializeGlobalState() does the exact same thing to
> other APZ preferences.

And regarding naming, all other APZ prefs are prefixed with 'apz.'.

	Benoit Girard (:BenWa)
	Comment 26 • 11 years ago

Those are bugs, so going forward we shouldn't make it worse.

	Botond Ballo [:botond] Assignee
	Comment 27 • 11 years ago

This is working now.

	Botond Ballo [:botond] Assignee
	Comment 28 • 11 years ago

(In reply to Benoit Girard (:BenWa) from comment #26)
> Those are bugs, so going forward we shouldn't make it worse.

We discussed this in person, summarizing for the record:
  - The APZCTreeManager constructor is called on the main thread, so reading 
    preferences from there is fine.
  - apz.* naming is fine for now, apz might become a module at some point anyways.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 30 • 11 years ago

Comment on attachment 8363950 [details] [diff] [review]
Part 9 - Add a content description field to FrameMetrics and populate it if the apz.printtree pref is on

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

::: gfx/layers/FrameMetrics.h
@@ +6,5 @@
>  #ifndef GFX_FRAMEMETRICS_H
>  #define GFX_FRAMEMETRICS_H
>  
>  #include <stdint.h>                     // for uint32_t, uint64_t
> +#include <string>

For completeness add a comment similar to the other includes

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 31 • 11 years ago

Comment on attachment 8363893 [details] [diff] [review]
Part 10 - Print the APZC tree for debugging

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

I have two concerns here: one is the thread safety with respect to TreeLog. Right now all the logging is done from the compositor thread but if we start adding log output in other functions which may run on other threads then would that be problematic? The other is the performance implications because it looks like we'll do a bunch of work to serialize stuff even if the pref is disabled. But I don't want to prematurely optimize this so I'm fine with leaving it for now and worrying about it if it becomes a problem.

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +40,5 @@
>  {
>    return gPrintApzcTree;
>  }
>  
> +gfx::TreeLog sApzcTreeLog("apzctree");

Assuming this patch needs to be rebased, since the GetApzcTreePrintPref just above is no longer in the queue

@@ +130,5 @@
>    Collect(mRootApzc, &apzcsToDestroy);
>    mRootApzc = nullptr;
>  
>    if (aRoot) {
> +    sApzcTreeLog << "[start]\n";

I agree with BenWa's comment about making [start] and [end] more descriptive. I probably would have gone with <apzc-tree> and </apzc-tree> myself but if you can make it look closer to the layers dump that's fine too.

	Botond Ballo [:botond] Assignee
	Comment 32 • 11 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> I have two concerns here: one is the thread safety with respect to TreeLog.
> Right now all the logging is done from the compositor thread but if we start
> adding log output in other functions which may run on other threads then
> would that be problematic? 

My intention was to use TreeLog just for the APZC tree logging for now. I can add a comment saying so. If we later want to log stuff from the main thread, I think we can further enhance the logging mechanism to provide the appropriate thread safety at that time.

> The other is the performance implications because
> it looks like we'll do a bunch of work to serialize stuff even if the pref
> is disabled.

Can you give an example of what would be serialized? Since 'TreeLog::operator<<(Whatever)' exits early if the pref is disabled, 'Log::operator<<(Whatever)', which is what would do any serialization, is never called. Nor do we compute FrameMetrics::mContentDescription if the pref is disabled.

> ::: gfx/layers/composite/APZCTreeManager.cpp
> @@ +40,5 @@
> >  {
> >    return gPrintApzcTree;
> >  }
> >  
> > +gfx::TreeLog sApzcTreeLog("apzctree");
> 
> Assuming this patch needs to be rebased, since the GetApzcTreePrintPref just
> above is no longer in the queue

Yep.

> @@ +130,5 @@
> >    Collect(mRootApzc, &apzcsToDestroy);
> >    mRootApzc = nullptr;
> >  
> >    if (aRoot) {
> > +    sApzcTreeLog << "[start]\n";
> 
> I agree with BenWa's comment about making [start] and [end] more
> descriptive. I probably would have gone with <apzc-tree> and </apzc-tree>
> myself but if you can make it look closer to the layers dump that's fine too.

Each line of the output is prefixed with "apzctree" (see the attached example output). Is it OK like that?

	Botond Ballo [:botond] Assignee
	Comment 33 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #32)
> My intention was to use TreeLog just for the APZC tree logging for now.

s/TreeLog/sApzcTreeLog. Of course other TreeLog instances can be created and used elsewhere.

	Bas Schouten (:bas.schouten)
	Comment 34 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #11)
> (In reply to Bas Schouten (:bas.schouten) from comment #10)
> > Comment on attachment 8363355 [details] [diff] [review]
> > Part 5 - Allow writing a log statement that does not end in a newline
> > 
> > Review of attachment 8363355 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Do you know whether the compiler will compile this into branchless code? I
> > suspect so but I'd like us to be sure.
> 
> Maybe we can add a MOZ_LIKELY around the condition?

We could also make it a template parameter? ;)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 35 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #32)
> My intention was to use TreeLog just for the APZC tree logging for now. I
> can add a comment saying so. If we later want to log stuff from the main
> thread, I think we can further enhance the logging mechanism to provide the
> appropriate thread safety at that time.

Sounds good.

> Can you give an example of what would be serialized? Since
> 'TreeLog::operator<<(Whatever)' exits early if the pref is disabled,
> 'Log::operator<<(Whatever)', which is what would do any serialization, is
> never called. Nor do we compute FrameMetrics::mContentDescription if the
> pref is disabled.

My bad, I misread the earlier patch. This is fine then.

> > I agree with BenWa's comment about making [start] and [end] more
> > descriptive. I probably would have gone with <apzc-tree> and </apzc-tree>
> > myself but if you can make it look closer to the layers dump that's fine too.
> 
> Each line of the output is prefixed with "apzctree" (see the attached
> example output). Is it OK like that?

That's fine by me.

	Botond Ballo [:botond] Assignee
	Comment 36 • 11 years ago

(In reply to Bas Schouten (:bas.schouten) from comment #34)
> (In reply to Botond Ballo [:botond] from comment #11)
> > (In reply to Bas Schouten (:bas.schouten) from comment #10)
> > > Comment on attachment 8363355 [details] [diff] [review]
> > > Part 5 - Allow writing a log statement that does not end in a newline
> > > 
> > > Review of attachment 8363355 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Do you know whether the compiler will compile this into branchless code? I
> > > suspect so but I'd like us to be sure.
> > 
> > Maybe we can add a MOZ_LIKELY around the condition?
> 
> We could also make it a template parameter? ;)

We could. It would complicate things a bit because you could no longer say things like "gfxDebug(NoNewline) << ...", we'd need to instead have a "gfxDebugNoNewline" macro that expands to the different type. I think MOZ_LIKELY is good enough, but if you feel strongly about it, feel free to say so and r-. ;)

	Botond Ballo [:botond] Assignee
	Comment 37 • 11 years ago

Address review comment. Carrying r+.

	Botond Ballo [:botond] Assignee
	Comment 38 • 11 years ago

Rebase and address review comment. Carrying r+.

	Botond Ballo [:botond] Assignee
	Comment 41 • 11 years ago

Update sample output to include SLGs.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 42 • 11 years ago

Comment on attachment 8364032 [details] [diff] [review]
Part 12 - Include the ScrollableLayerGuid in APZC tree log

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

::: gfx/layers/FrameMetrics.h
@@ +408,5 @@
>  };
>  
> +template <int LogLevel>
> +gfx::Log<LogLevel>& operator<<(gfx::Log<LogLevel>& log, const ScrollableLayerGuid& aGuid) {
> +  return log << '(' << aGuid.mLayersId << ", " << aGuid.mPresShellId << ", " << aGuid.mScrollId << ')';

Actually, I'd prefer if you removed the spaces after the commas. Not just for consistency with printing other objects but also it's more visually grouped and easier to grab in awk scripts (which I tend to use not infrequently on log output).

	Boris Zbarsky [:bzbarsky]
	Comment 43 • 11 years ago

Comment on attachment 8363911 [details] [diff] [review]
Part 8 - Add a Describe() method to nsIContent

r=me

	Botond Ballo [:botond] Assignee
	Comment 44 • 11 years ago

Updated part 12 patch to remove spaces between the SLG components. Carrying r+.

	Botond Ballo [:botond] Assignee
	Comment 45 • 10 years ago

These patches cause a linker error "undefined reference to 'GetGFX2DLog()'" on Fennec. Not sure why yet. Possibly including Logging.h from FrameMetrics.h changes whether or not PR_LOGGING is defined inside Logging.h.

	Botond Ballo [:botond] Assignee
	Comment 46 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #45)
> These patches cause a linker error "undefined reference to 'GetGFX2DLog()'"
> on Fennec. Not sure why yet. Possibly including Logging.h from
> FrameMetrics.h changes whether or not PR_LOGGING is defined inside Logging.h.

The problem was that both the declaration of GetGFX2DLog() in Logging.h, and its definition in Factory.cpp, were guarded by '#ifdef PR_LOGGING'. When building on Fennec, PR_LOGGING is not defined by the build system, but it is defined in prlog.h, and LayersTypes.h includes prlog.h if DEBUG is defined. LayersTypes.h is not included by Factory.cpp, but is by ContentClient.cpp, which is in the same unified source file as APZCTreeManager.cpp, which now includes Logging.h. As a result, PR_LOGGING was being defined in Logging.h, but not in Factory.cpp, so the declaration of GetGFX2DLog() was being compiled but not the definition, and we got the linker error.

The attached patch resolves the problem by adjusting the guards for the declaration and definition of GetGFX2DLog() to be '#if defined(DEBUG) || defined(PR_LOGGING)".

	Botond Ballo [:botond] Assignee
	Comment 47 • 10 years ago

Try run: https://tbpl.mozilla.org/?tree=Try&rev=7e752884e09e

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 48 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #46)
> The attached patch resolves the problem by adjusting the guards for the
> declaration and definition of GetGFX2DLog() to be '#if defined(DEBUG) ||
> defined(PR_LOGGING)".

I don't know if the standalone-ness of Moz2D will allow it but I would have preferred explicitly including prlog.h in both Logging.h and Factory.cpp.

(In reply to Botond Ballo [:botond] from comment #47)
> Try run: https://tbpl.mozilla.org/?tree=Try&rev=7e752884e09e

Looks like the compiler didn't like you trying to outsmart it :p

	Botond Ballo [:botond] Assignee
	Comment 49 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #48)
> (In reply to Botond Ballo [:botond] from comment #46)
> > The attached patch resolves the problem by adjusting the guards for the
> > declaration and definition of GetGFX2DLog() to be '#if defined(DEBUG) ||
> > defined(PR_LOGGING)".
> 
> I don't know if the standalone-ness of Moz2D will allow it but I would have
> preferred explicitly including prlog.h in both Logging.h and Factory.cpp.

Bas, is it OK to unconditionally include prlog.h from gfx/2d/Logging.h?

	Botond Ballo [:botond] Assignee
	Comment 50 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #48)
> (In reply to Botond Ballo [:botond] from comment #47)
> > Try run: https://tbpl.mozilla.org/?tree=Try&rev=7e752884e09e
> 
> Looks like the compiler didn't like you trying to outsmart it :p

Updated Part 9 patch to be a little less smart and avoid the unused variable error. Carrying r+.

	Botond Ballo [:botond] Assignee
	Comment 51 • 10 years ago

New try push: https://tbpl.mozilla.org/?tree=Try&rev=2f7504009e4f

	Botond Ballo [:botond] Assignee
	Comment 52 • 10 years ago

(In reply to Botond Ballo [:botond] [c++ standards meeting Feb 10-15] from comment #51)
> New try push: https://tbpl.mozilla.org/?tree=Try&rev=2f7504009e4f

Unfortunately we still have a build error on Windows XP, because nsGlobalWindow.cpp includes (indirectly) FrameMetrics.h, which now includes Logging.h, which includes windows.h, but nsGlobalWindow.cpp asserts [1] that windows.h is not included.

I'm not sure what the purpose of preventing windows.h from being included from nsGlobalWindow.cpp is, but assuming there's a good reason for that, we can work around this by moving the inline definition of OutputMessage() in Logging.h (which uses a Windows API on Windows) out of line into cpp file. Bas, can we do this?

[1] http://hg.mozilla.org/mozilla-central/annotate/879038dcacb7/dom/base/nsGlobalWindow.cpp#l13491

	Botond Ballo [:botond] Assignee
	Comment 53 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #52)
> Unfortunately we still have a build error on Windows XP, because
> nsGlobalWindow.cpp includes (indirectly) FrameMetrics.h, which now includes
> Logging.h, which includes windows.h, but nsGlobalWindow.cpp asserts [1] that
> windows.h is not included.
> 
> I'm not sure what the purpose of preventing windows.h from being included
> from nsGlobalWindow.cpp is, but assuming there's a good reason for that, we
> can work around this by moving the inline definition of OutputMessage() in
> Logging.h (which uses a Windows API on Windows) out of line into cpp file.
> Bas, can we do this?

The attached patch does this.

One more try push: https://tbpl.mozilla.org/?tree=Try&rev=10818e37426e

	Bas Schouten (:bas.schouten)
	Comment 54 • 10 years ago

Comment on attachment 8378061 [details] [diff] [review]
Part -1 - Define OutputMessage() in Factory.cpp rather than Logging.h

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

I'd rather not if we want to be able to do fast release build logging. I'd hate to add a needless extra function call overhead. As a hack we could just manually declare OutputMessage?

	Botond Ballo [:botond] Assignee
	Comment 55 • 10 years ago

(In reply to Bas Schouten (:bas.schouten) from comment #54)
> I'd rather not if we want to be able to do fast release build logging. I'd
> hate to add a needless extra function call overhead. As a hack we could just
> manually declare OutputMessage?

I assume you meant here manually declare OutpuDebugStringA, which the only API we use from windows.h, instead of including windows.h. The attached patch does that.

	Botond Ballo [:botond] Assignee
	Comment 56 • 10 years ago

One more Try: https://tbpl.mozilla.org/?tree=Try&rev=810211094cbe

	Botond Ballo [:botond] Assignee
	Comment 57 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #56)
> One more Try: https://tbpl.mozilla.org/?tree=Try&rev=810211094cbe

Silly me. If windows.h isn't included, then WINAPI and LPCSTR, which appear in the signature of my "manually-declared" OutputDebugStringA, aren't defined either. I need to use their expansions directly.

Trying again: https://tbpl.mozilla.org/?tree=Try&rev=3c4651d030ad

	Botond Ballo [:botond] Assignee
	Comment 58 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #57)
> Trying again: https://tbpl.mozilla.org/?tree=Try&rev=3c4651d030ad

And failed again, something about linkage this time. I'm getting tired of this. I'll set up a local Windows XP build and get it to compile locally there.

	Botond Ballo [:botond] Assignee
	Comment 59 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #58)
> (In reply to Botond Ballo [:botond] from comment #57)
> > Trying again: https://tbpl.mozilla.org/?tree=Try&rev=3c4651d030ad
> 
> And failed again, something about linkage this time. I'm getting tired of
> this. I'll set up a local Windows XP build and get it to compile locally
> there.

That turned out to be a good idea - I had to go through another four edit/compile cycles on my local build before getting it to compile. For the record, the magic incantations were 'extern "C"' and '__declspec(dllimport)' at the beginning of the declaration of OutputDebugStringA, in that order.

Unfortunately, it still does not link, because it seems Factory.cpp, which provides the definitions for some things defined in Logging.h, does not get linked on Windows XP. In fact, it seems the entire gfx/2d module is not linked on Windows XP. Prior to my patches, this was OK because gfx/2d/Logging.h was only included by things in the gfx/2d module. However, my patches introduce the use of gfx/2d/Logging.h from other modules. I will have to discuss this with Bas to arrive at a solution.

	Botond Ballo [:botond] Assignee
	Comment 60 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #46)
> The attached patch resolves the problem by adjusting the guards for the
> declaration and definition of GetGFX2DLog() to be '#if defined(DEBUG) ||
> defined(PR_LOGGING)".

Unfortunately, this breaks APZC tree printing in debug builds because the messages now go the PR log...

	Botond Ballo [:botond] Assignee
	Comment 61 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #59)
> Unfortunately, it still does not link, because it seems Factory.cpp, which
> provides the definitions for some things defined in Logging.h, does not get
> linked on Windows XP. In fact, it seems the entire gfx/2d module is not
> linked on Windows XP. Prior to my patches, this was OK because
> gfx/2d/Logging.h was only included by things in the gfx/2d module. However,
> my patches introduce the use of gfx/2d/Logging.h from other modules. I will
> have to discuss this with Bas to arrive at a solution.

So it turns out gfx/2d _is_ being linked on Windows XP, but into a separate 'gkmedia' library rather than into xul. As a result, things defined in gfx/2d but used outside of it (e.g. sGfxLogLevel) need to marked with GFX2D_API, which expands to __declspec(dllexport) inside gfx/2d, and __declspec(dllimport) elsewhere.

Once I added that I ran into an additional problem where the expansion of GFX2D_API is conditioned on MOZ_GFX, which is defined only in gfx/2d. I changed it to be conditioned on GKMEDIAS_SHARED_LIBRARY, which is defined whenever we are creating this separate 'gkmedia' library (and adjusted configure.in accordingly to make the symbol GKMEDIAS_SHARED_LIBRARY available to source code), and that worked fine.

I ran into one final issue, where a function that shouldn't have been marked GFX2D_API because it was defined inline (http://mxr.mozilla.org/mozilla-central/source/gfx/2d/Rect.h?force=1#100), was. I corrected this, and finally got the result to compile and link on Windows XP.

The attached patch consolidates the current Part -1, Part 0, and Part 1 patches, and includes the changes I just describes, since all of these concern allowing gfx/2d/Logging.h to be used outside of gfx/2d.

New Try push: https://tbpl.mozilla.org/?tree=Try&rev=a2ace943c5f3

Note that the issue I raised in comment #60 remains to be resolved.

	Botond Ballo [:botond] Assignee
	Comment 62 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #60)
> (In reply to Botond Ballo [:botond] from comment #46)
> > The attached patch resolves the problem by adjusting the guards for the
> > declaration and definition of GetGFX2DLog() to be '#if defined(DEBUG) ||
> > defined(PR_LOGGING)".
> 
> Unfortunately, this breaks APZC tree printing in debug builds because the
> messages now go the PR log...

Fixed this for now by adjusting the preprocessor conditions in OutputMessage() such that on b2g and android, we always use logcat rather than the PR log. In the longer term we can get the PR log to play more nicely with b2g/android.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=cd1b3dd2482b

	Bas Schouten (:bas.schouten)
	Comment 63 • 10 years ago

Comment on attachment 8379913 [details] [diff] [review]
Part 1 - Allow gfx/2d/Logging.h to be used outside of gfx/2d

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

Looks fine to me.

	Botond Ballo [:botond] Assignee
	Comment 64 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #62)
> New try push: https://tbpl.mozilla.org/?tree=Try&rev=cd1b3dd2482b

Unfortunately, this is showing errors on some Linux and OS X platforms, such as:

package-manifest:40: Missing file(s): bin/libgkmedias.so

I'm pretty sure this is related to the configure.in change I made (making GKMEDIAS_SHARED_LIBRARY available to source code), though I'm not quite sure how...

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 65 • 10 years ago

Comment on attachment 8379913 [details] [diff] [review]
Part 1 - Allow gfx/2d/Logging.h to be used outside of gfx/2d

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

::: configure.in
@@ +7715,5 @@
>  if test "$OS_ARCH" = "WINNT"; then
>    GKMEDIAS_SHARED_LIBRARY=1
>  fi
>  AC_SUBST(GKMEDIAS_SHARED_LIBRARY)
> +AC_DEFINE(GKMEDIAS_SHARED_LIBRARY)

I think you might want to move this AC_DEFINE inside the WINNT check above.

	Botond Ballo [:botond] Assignee
	Comment 66 • 10 years ago
landing try

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #65)
> ::: configure.in
> @@ +7715,5 @@
> >  if test "$OS_ARCH" = "WINNT"; then
> >    GKMEDIAS_SHARED_LIBRARY=1
> >  fi
> >  AC_SUBST(GKMEDIAS_SHARED_LIBRARY)
> > +AC_DEFINE(GKMEDIAS_SHARED_LIBRARY)
> 
> I think you might want to move this AC_DEFINE inside the WINNT check above.

That did the trick. Thanks!

New try push is clean: https://tbpl.mozilla.org/?tree=Try&rev=ca797da79dc6

Landed: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=63d96f3e3e42

	Ed Morley [:emorley]
	Comment 67 • 10 years ago
backout

(In reply to Botond Ballo [:botond] from comment #66)
> Landed:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?changeset=63d96f3e3e42

Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35223589&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=ecfb4d63943f

	Botond Ballo [:botond] Assignee
	Comment 68 • 10 years ago

(In reply to Ed Morley [:edmorley UTC+0] from comment #67)
> (In reply to Botond Ballo [:botond] from comment #66)
> > Landed:
> > https://hg.mozilla.org/integration/mozilla-inbound/
> > pushloghtml?changeset=63d96f3e3e42
> 
> Backed out for:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=35223589&tree=Mozilla-
> Inbound

Right. Try build was clean, but it was optimized builds only. I still have a linker error for the Windows XP _debug_ build... Sorry about that.

	Botond Ballo [:botond] Assignee
	Comment 69 • 10 years ago
try

(In reply to Botond Ballo [:botond] from comment #68)
> Try build was clean, but it was optimized builds only. I still have a
> linker error for the Windows XP _debug_ build... Sorry about that.

I needed a GFX2D_API in front of GetGFX2DLog(), similarly to how I needed one in front of sGfxLogLevel.

Updated Part 1 patch, carrying r+.

One more try build, this time for both debug and release builds, to be absolutely sure nothing is busted: https://tbpl.mozilla.org/?tree=Try&rev=7d7ad2d1ffdf.

	Botond Ballo [:botond] Assignee
	Comment 70 • 10 years ago
landing

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=e4c0e3d3a768

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 71 • 10 years ago
backout

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/c9f4f70e46e1 because this seems to have turned several Android tests permanently orange (they were starred as Bug 912238 on your try push, but this seems to have made that failure happen constantly):

Android 4.0 mochitest-6, mochitest-7 and tspaint all turned orange because of this push:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35252067&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35251554&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35250076&tree=Mozilla-Inbound

robocheck-2 failed in a similar way once, but hasn't since: https://tbpl.mozilla.org/php/getParsedLog.php?id=35250554&tree=Mozilla-Inbound

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 72 • 10 years ago

Might as well fix bug 912238 properly then.

	Botond Ballo [:botond] Assignee
	Comment 73 • 10 years ago

ReTrying after bug 912238 landing: https://tbpl.mozilla.org/?tree=Try&rev=b5d023b54a1b

	Botond Ballo [:botond] Assignee
	Comment 74 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #73)
> ReTrying after bug 912238 landing:
> https://tbpl.mozilla.org/?tree=Try&rev=b5d023b54a1b

We are still seeing some Talos failures. I did some targeted Try pushes to bisect the patchset:

https://tbpl.mozilla.org/?tree=Try&rev=153ab7477ea4
https://tbpl.mozilla.org/?tree=Try&rev=0b9b2d76d41f
https://tbpl.mozilla.org/?tree=Try&rev=6ae5ecc17b02
https://tbpl.mozilla.org/?tree=Try&rev=3b785af1de27
https://tbpl.mozilla.org/?tree=Try&rev=745452e62bd2

They suggest that the problem is caused by the Part 10 patch, which actually adds the tree logging to APZCTreeManager. Given that the problem seems to be with the shutdown sequence, the culprit is usually the static variable 'gfx::TreeLog sApzcTreeLog' that the patch introduces, whose destructor flushes the log. Perhaps by the time the destructor is called, it's too late in the shutdown sequence to be doing things like that?

	Botond Ballo [:botond] Assignee
	Comment 75 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #74)
> We are still seeing some Talos failures. I did some targeted Try pushes to
> bisect the patchset:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=153ab7477ea4
> https://tbpl.mozilla.org/?tree=Try&rev=0b9b2d76d41f
> https://tbpl.mozilla.org/?tree=Try&rev=6ae5ecc17b02
> https://tbpl.mozilla.org/?tree=Try&rev=3b785af1de27
> https://tbpl.mozilla.org/?tree=Try&rev=745452e62bd2
> 
> They suggest that the problem is caused by the Part 10 patch, which actually
> adds the tree logging to APZCTreeManager. Given that the problem seems to be
> with the shutdown sequence, the culprit is probably the static variable
> 'gfx::TreeLog sApzcTreeLog' that the patch introduces, whose destructor
> flushes the log. Perhaps by the time the destructor is called, it's too late
> in the shutdown sequence to be doing things like that?

Updated Part 10 patch to make the tree log a member of APZCTreeManager rather than a static storage duration object. This ensures that its destructor is called earlier in the shutdown sequence.

This Try push confirms that the Talos failures are gone: https://tbpl.mozilla.org/?tree=Try&rev=34136a76b430

Note that Kats has already reviewed this patch; the only change since then is the tree log being changed from a static variable to a member of APZCTreeManager.

	Benoit Girard (:BenWa)
	Comment 76 • 10 years ago

Comment on attachment 8384218 [details] [diff] [review]
Part 10 - Print the APZC tree for debugging

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

::: gfx/layers/composite/APZCTreeManager.h
@@ +340,5 @@
>     * been scrolled as far as it can, any overscroll will be handed off to
>     * the next APZC in the chain.
>     */
>    Vector< nsRefPtr<AsyncPanZoomController> > mOverscrollHandoffChain;
> +  /* For logging the APZC tree for debugging (enabled by th e apz.printtree

typo, the

	Botond Ballo [:botond] Assignee
	Comment 77 • 10 years ago

Relanded again: https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=0ed4786b22de

	Ryan VanderMeulen [:RyanVM]
	Comment 78 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/1e198a326d40
https://hg.mozilla.org/mozilla-central/rev/6d773a03f680
https://hg.mozilla.org/mozilla-central/rev/f6a8fd3cc486
https://hg.mozilla.org/mozilla-central/rev/5de6d97667b0
https://hg.mozilla.org/mozilla-central/rev/e4c8a1eb2975
https://hg.mozilla.org/mozilla-central/rev/210e09a2d111
https://hg.mozilla.org/mozilla-central/rev/8a1799d0ec17
https://hg.mozilla.org/mozilla-central/rev/6d0217b266f6
https://hg.mozilla.org/mozilla-central/rev/3da14f8555f7
https://hg.mozilla.org/mozilla-central/rev/12eceba2ed2b
https://hg.mozilla.org/mozilla-central/rev/62b54b8fa885
https://hg.mozilla.org/mozilla-central/rev/0ed4786b22de

	(no longer active)
	Comment 79 • 10 years ago

One hunk from the part 1 of this patch conflicted with bug 978776, which I fixed here:

<https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf800d6a85e>

FWIW, moz2d is now in libxul, and GKMEDIAS_SHARED_LIBRARY is the wrong condition to test for here since it tells you whether gkmedias is built as a shared library, not whether moz2d is inside it.