Closed Bug 1222244 Opened 9 years ago Closed 8 years ago

Replace PRLogModuleInfo usage with LazyLogModule

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 55.0

People

(Reporter: rkent, Unassigned)

References

(
URL
)

Details

User Story

Eric Rahm erahm at mozilla.com
Wed Oct 28 23:35:27 UTC 2015

Hi All-

I recently landed a lazily-loaded log module class, LazyLogModule [1],  suitable for declaring static references to log modules in a thread-safe way. 

Currently this class is opt-in and PRLogModuleInfo can still be used w/ MOZ_LOG. But be forewarned, as we move forward to a glorious world where log levels can be dynamically updated w/o restarting the browser, PRLogModuleInfo will be left behind and only LogModule instances will get the benefit.

Before:
=======

  PRLogModuleInfo*
  GetMyLog()
  {
    static PRLogModuleInfo* sLogModule = PR_NewLogModule("MyLog");
    return sLogModule;
  }

  void Foo() { MOZ_LOG(GetMyLog(), ...); }

Now:
====

  LazyLogModule sMyLogModule("MyLog");

  void Foo() { MOZ_LOG(sMyLogModule, ...); }

The Future:
===========

We have already converted xpcom over to using it [2] and are quite happy with how things turned out.

This is where you come in dear reader!

Please switch over your PRLogModuleInfo instances to LazyLogModule. I have a tracking bug [3] for the overall code base and have split out bugs for smaller chunks. If you intend to help out just go ahead and assign yourself one of those bugs!

Addendum on Thread Safety
=========================

There are some common mistakes that TSan runs are tripping over, such as:

// Not-quite-right use of static initialization
GetLog() {
  static* myLog = nullptr; // This is thread-safe on modern compilers
  if (!myLog)
    myLog = ... // This is not.
}

// Global, initialize wherever it's used
static PRLogModuleInfo* myLog;

Foo::Foo() {
  if (!myLog)
    myLog = ...
}

And to round it out, PR_NewLogModule is not thread-safe [4].

-e

[1] https://dxr.mozilla.org/mozilla-central/rev/fc706d376f0658e560a59c3dd520437b18e8c4a4/xpcom/base/Logging.h#100
[2] https://hg.mozilla.org/mozilla-central/rev/f9cf413cb3da
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1219461
[4] https://bugzilla.mozilla.org/show_bug.cgi?id=1073578

Attachments

(2 files, 1 obsolete file)

Temporary bustage fix for nsCMS.cpp nsCMSSecureMessage.cpp [checked in]
1.27 KB, patch
rkent
: review+
Details | Diff | Splinter Review
Replace PRLogModuleInfo usage with LazyLogModule (v2).
51.91 KB, patch
jorgk-bmo
: review+
aceman
: review+
aceman
: feedback+
Details | Diff | Splinter Review
See post in m.d.platform
Minimal/temp fix just to get the tree building again.
Attachment #8730645 - Flags: review?(rkent)
Comment on attachment 8730645 [details] [diff] [review] Temporary bustage fix for nsCMS.cpp nsCMSSecureMessage.cpp [checked in] Review of attachment 8730645 [details] [diff] [review]: ----------------------------------------------------------------- I can't get this to build for unrelated reasons, but it appears to be the correct thing to do. This is only a piece of the larger bug, so leave-open
Attachment #8730645 - Flags: review?(rkent) → review+
There are many more instances of PRLogModuleInfo in c-c, are those not affected?
https://hg.mozilla.org/comm-central/rev/89caadb262baf1e4c8a300a65aff4ec40b864123 Bug 1222244 - Replace PRLogModuleInfo usage with LazyLogModule, nsCMS part. r=rkent a=Windows bustage fix CLOSED TREE DONTBUILD
Attachment #8730645 - Attachment description: Temporary bustage fix for nsCMS.cpp nsCMSSecureMessage.cpp → Temporary bustage fix for nsCMS.cpp nsCMSSecureMessage.cpp [checked in]
Blocks: 1219461
URL: https://lists.mozilla.org/pipermail/d...
User Story: (updated)
Heads up, I landed remove of prlog.h in m-c, that will probably break thunderbird. The attached patch is a rough draft of changes swapping c-c over to LazyLogModule.
Sorry I should have mentioned that as per usual this is untested. If someone from the the thunderbird team can finish it off that would be great!
Thanks, we'll finish it off.
The patch still fails. With my no knowledge of C++ it's too high for me to fix. Jörg, please can you look at it?
Compiling locally and shipped off to try before reading Richard's comment: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=31978ee67e55943ccbf31172d58ed4cdb68c67fe Well, I'll cancel that. I'll get it fixed.
A few things you might run into: Build fails due to: - missing PR_ASSERT - solution: switch to MOZ_ASSERT - missing PR_BEGIN_MACRO / PR_END_MACRO - solution: switch to do { / } while(0) *or* #include "prtypes.h" - missing PR_LogPrint - solution: use MOZ_LOG instead, or possibly printf_stderr - missing PR_LOGGING - solution: just remove the #ifdef (but keep the code in it), PR_LOGGING was always defined
Failed due to: c:/mozilla-source/comm-central/mailnews/import/winlivemail/nsWMImport.cpp(39): error C2374: 'WMLOGMODULE': redefinition; multiple initialization c:/mozilla-source/comm-central/mailnews/import/outlook/src/nsOutlookImport.cpp(40): error C2374: 'OUTLOOKLOGMODULE': redefinition; multiple initialization Solution: remove that line ;-) A few cases so far, I believe it's already done in ImportDebug.h included by the others.
A few more errors to address, getting there, but don't hold your breath ;-)
This compiles and links. I made all LazyLogModule static, since there were a few link errors otherwise.
Attachment #8854644 - Attachment is obsolete: true
Attachment #8855034 - Flags: review+
I've confirmed with Eric on IRC that my modifications were right: - all static - fix as per comment 14, remove second definition from .cpp file. - remove POP3LOGMODULE->name and corresponding %s. Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8854644&action=interdiff&newid=8855034&headers=1
https://hg.mozilla.org/comm-central/rev/8d998c1d1b3aebcb6165a36847cd5c3dc4bf74d3 If there are any issues, I'll land a follow-up ;-) IRC: jorgk so if it compiles and links, it should work, right? erahm jorgk: yep! jorgk OK, done then, thanks again!!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Comment on attachment 8855034 [details] [diff] [review] Replace PRLogModuleInfo usage with LazyLogModule (v2). Review of attachment 8855034 [details] [diff] [review]: ----------------------------------------------------------------- Thanks looks nice. It also builds for me with the breaking change from m-c. I do not understand the capitalisation in some of the module names, but we probably can't fix that easily due to existing users. ::: mailnews/db/msgdb/src/nsMailDatabase.cpp @@ +15,5 @@ > #include "prprf.h" > #include "nsMsgUtils.h" > #include "nsIMsgPluggableStore.h" > > +static mozilla::LazyLogModule IMAPOffline("IMAPOFFLINE"); Hopefully this is only used for the IMAP log (as it is placed in generic nsMailDatabase.cpp). ::: mailnews/db/msgdb/src/nsMsgOfflineImapOperation.cpp @@ +9,5 @@ > #include "mozilla/Logging.h" > > using namespace mozilla; > > +static LazyLogModule IMAPOffline("IMAPOFFLINE"); Is if fine that this is already defined elsewhere? ::: mailnews/local/src/nsPop3Protocol.cpp @@ +50,5 @@ > > using namespace mozilla; > > +static mozilla::LazyLogModule POP3LOGMODULE("POP3"); > +#define POP3LOG(str) "[this=%p] " str, this Why the change to POP3LOG ? ::: mailnews/local/src/nsPop3Sink.cpp @@ +43,5 @@ > #include "nsIScriptError.h" > #include "nsIConsoleService.h" > > +static mozilla::LazyLogModule POP3LOGMODULE("POP3"); > +#define POP3LOG(str) "sink: [this=%p] " str, this Why the change to POP3LOG ?
Attachment #8855034 - Flags: feedback+
Thanks for looking this over. (In reply to :aceman from comment #19) > ::: mailnews/db/msgdb/src/nsMailDatabase.cpp > > +static mozilla::LazyLogModule IMAPOffline("IMAPOFFLINE"); > Hopefully this is only used for the IMAP log (as it is placed in generic > nsMailDatabase.cpp). Well, it was like that. - extern PRLogModuleInfo *IMAPOffline; > > +static LazyLogModule IMAPOffline("IMAPOFFLINE"); > Is if fine that this is already defined elsewhere? Yes, if it's static. > Why the change to POP3LOG ? See comment #17: remove POP3LOGMODULE->name and corresponding %s since |->name| doesn't compile and MOZ_LOG logs the module anyway.
(In reply to :aceman from comment #19) > Thanks looks nice. > It also builds for me with the breaking change from m-c. > I do not understand the capitalisation in some of the module names, but we > probably can't fix that easily due to existing users. Actually it seems this already changes the keywords that can be passed to MOZ_LOG_MODULES. I used pop3:5 before and it no longer works with the patch. I needed POP3:5. So it seems now would be a good time to decide on good capitalizations of the log module name and update the doc pages which reference the lowercase names. > ::: mailnews/local/src/nsPop3Protocol.cpp > @@ +50,5 @@ > > > > using namespace mozilla; > > > > +static mozilla::LazyLogModule POP3LOGMODULE("POP3"); > > +#define POP3LOG(str) "[this=%p] " str, this > > Why the change to POP3LOG ? Ok, I see, the new logger output log module name automatically (and also thread ID).
(In reply to :aceman from comment #22) > I used pop3:5 before and it no longer works with the patch. I needed POP3:5. > So it seems now would be a good time to decide on good capitalizations of > the log module name and update the doc pages which reference the lowercase > names. All yours in another bug. My job description says: "Maintain comm-central ... to keep Thunderbird buildable". That's done.
Comment on attachment 8855034 [details] [diff] [review] Replace PRLogModuleInfo usage with LazyLogModule (v2). So this is fine, but we need to sort out the module names.
Attachment #8855034 - Flags: review+
Depends on: 1353919
(In reply to Jorg K (GMT+2) from comment #18) > https://hg.mozilla.org/comm-central/rev/8d998c1d1b3aebcb6165a36847cd5c3dc4bf74d3 > > If there are any issues, I'll land a follow-up ;-) > > IRC: > jorgk so if it compiles and links, it should work, right? > erahm jorgk: yep! > jorgk OK, done then, thanks again!! and set Status/Resolution to RESOLVED FIXED Hm, I wonder if it's a coincidence that SeaMonkey stopped building immediately after the following nightly (i.e. no hourlies since then): 20170405003004 https://hg.mozilla.org/comm-central/rev/cb74ea57509304cec329ec5c7811b45aaadf2524 https://hg.mozilla.org/mozilla-central/rev/b043233ec04f
Blocks: 1353919
No longer depends on: 1353919
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: