Closed Bug 958596 Opened 6 years ago Closed 6 years ago

Add logging code for debugging the APZC tree

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(14 files, 17 obsolete files)

15.31 KB, patch
Details | Diff | Splinter Review
1.20 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
2.45 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
2.08 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.18 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.80 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
5.41 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.83 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.37 KB, text/plain
Details
4.63 KB, patch
botond
: review+
Details | Diff | Splinter Review
10.12 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.54 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
5.09 KB, patch
botond
: review+
Details | Diff | Splinter Review
7.31 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
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
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.
Assignee: nobody → botond
Attachment #8358505 - Attachment description: bug958596.patch → Original out-of-tree implementation
Attachment #8363350 - Flags: review?(bas)
Attachment #8363357 - Flags: review?(bas)
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.
Attachment #8363350 - Flags: review?(bas) → review+
Attachment #8363351 - Flags: review?(bas) → review+
Attachment #8363352 - Flags: review?(bas) → review+
Attachment #8363354 - Flags: review?(bas) → review+
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.
Attachment #8363355 - Flags: review?(bas) → review+
See Also: → 962624
(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?
Attachment #8363358 - Attachment is obsolete: true
Attachment #8363890 - Flags: review?(bzbarsky)
Attachment #8363891 - Flags: review?(tnikkel)
Attachment #8363891 - Flags: review?(bugmail.mozilla)
Attachment #8363891 - Flags: feedback?(bgirard)
Attachment #8363893 - Flags: review?(bugmail.mozilla)
Attachment #8363893 - Flags: feedback?(bgirard)
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.
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.
Attachment #8363891 - Flags: review?(tnikkel) → review+
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.
Attachment #8363890 - Flags: review?(bzbarsky) → review-
(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.
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.
Attachment #8363891 - Attachment is obsolete: true
Attachment #8363891 - Flags: review?(bugmail.mozilla)
Attachment #8363891 - Flags: feedback?(bgirard)
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.
Attachment #8363890 - Attachment is obsolete: true
Attachment #8363909 - Flags: review?(bzbarsky)
Whoops, forgot to update one part. Fixed.
Attachment #8363909 - Attachment is obsolete: true
Attachment #8363909 - Flags: review?(bzbarsky)
Attachment #8363911 - Flags: review?(bzbarsky)
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.
Attachment #8363893 - Flags: feedback?(bgirard) → feedback-
(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.
(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.'.
Those are bugs, so going forward we shouldn't make it worse.
This is working now.
Attachment #8363950 - Flags: review?(tnikkel)
Attachment #8363950 - Flags: review?(bugmail.mozilla)
Attachment #8363950 - Flags: feedback?(bgirard)
(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.
Attached file Sample output of APZC tree in logcat (obsolete) —
Attachment #8363950 - Flags: review?(tnikkel) → review+
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
Attachment #8363950 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8363893 - Flags: review?(bugmail.mozilla) → review+
(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?
(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.
(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? ;)
(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.
(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-. ;)
Address review comment. Carrying r+.
Attachment #8363950 - Attachment is obsolete: true
Attachment #8363950 - Flags: feedback?(bgirard)
Attachment #8364029 - Flags: review+
Rebase and address review comment. Carrying r+.
Attachment #8363893 - Attachment is obsolete: true
Attachment #8364030 - Flags: review+
Attachment #8364032 - Flags: review?(bugmail.mozilla)
Update sample output to include SLGs.
Attachment #8363957 - Attachment is obsolete: true
Attachment #8364032 - Flags: review?(bugmail.mozilla) → review+
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).
Comment on attachment 8363911 [details] [diff] [review]
Part 8 - Add a Describe() method to nsIContent

r=me
Attachment #8363911 - Flags: review?(bzbarsky) → review+
Attachment #8364031 - Flags: review?(bas) → review+
Updated part 12 patch to remove spaces between the SLG components. Carrying r+.
Attachment #8364032 - Attachment is obsolete: true
Attachment #8364438 - Flags: review+
Blocks: 962624
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.
Attachment #8363889 - Flags: review?(bas) → review+
Attachment #8363357 - Flags: review?(bas) → review+
(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)".
Attachment #8372981 - Flags: review?(bas)
(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
(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?
Flags: needinfo?(bas)
(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+.
Attachment #8364029 - Attachment is obsolete: true
Attachment #8375011 - Flags: review+
(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
(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
Attachment #8378061 - Flags: review?(bas)
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?
(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.
Attachment #8378061 - Attachment is obsolete: true
Attachment #8378061 - Flags: review?(bas)
Attachment #8378414 - Flags: review?(bas)
(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
Attachment #8378414 - Attachment is obsolete: true
Attachment #8378414 - Flags: review?(bas)
Attachment #8378499 - Flags: review?(bas)
(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.
Attachment #8378499 - Flags: review?(bas) → review+
(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.
(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...
(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.
Attachment #8363350 - Attachment is obsolete: true
Attachment #8372981 - Attachment is obsolete: true
Attachment #8378499 - Attachment is obsolete: true
Attachment #8372981 - Flags: review?(bas)
(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
Attachment #8363351 - Attachment is obsolete: true
Attachment #8379930 - Flags: review?(bas)
Attachment #8379913 - Flags: review?(bas)
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.
Attachment #8379913 - Flags: review?(bas) → review+
Attachment #8379930 - Flags: review?(bas) → review+
(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...
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.
(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
Flags: needinfo?(bas)
(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.
(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.
Attachment #8379913 - Attachment is obsolete: true
Attachment #8381636 - Flags: review+
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
Might as well fix bug 912238 properly then.
Depends on: 912238
(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?
(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.
Attachment #8364030 - Attachment is obsolete: true
Attachment #8384218 - Flags: review?(bgirard)
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
Attachment #8384218 - Flags: review?(bgirard) → review+
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.
Depends on: 981607
Depends on: 1030115
You need to log in before you can comment on or make changes to this bug.