Closed
Bug 1222244
Opened 9 years ago
Closed 8 years ago
Replace PRLogModuleInfo usage with LazyLogModule
Categories
(MailNews Core :: Backend, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 55.0
People
(Reporter: rkent, Unassigned)
References
()
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)
(deleted),
patch
|
rkent
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorgk-bmo
:
review+
aceman
:
review+
aceman
:
feedback+
|
Details | Diff | Splinter Review |
See post in m.d.platform
Comment 2•9 years ago
|
||
Minimal/temp fix just to get the tree building again.
Attachment #8730645 -
Flags: review?(rkent)
Reporter | ||
Comment 3•9 years ago
|
||
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+
Comment 4•9 years ago
|
||
There are many more instances of PRLogModuleInfo in c-c, are those not affected?
Comment 5•9 years ago
|
||
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
Updated•9 years ago
|
Attachment #8730645 -
Attachment description: Temporary bustage fix for nsCMS.cpp nsCMSSecureMessage.cpp → Temporary bustage fix for nsCMS.cpp nsCMSSecureMessage.cpp [checked in]
Comment 6•9 years ago
|
||
(In reply to Kent James (:rkent) from comment #0)
> See post in m.d.platform
http://codeverge.com/mozilla.dev.platform/lazylogmodule-a-thread-safe-replacement/2038682
Updated•9 years ago
|
User Story: (updated)
Comment 7•8 years ago
|
||
Comment 8•8 years ago
|
||
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.
Comment 9•8 years ago
|
||
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!
Comment 10•8 years ago
|
||
Thanks, we'll finish it off.
Comment 11•8 years ago
|
||
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?
Comment 12•8 years ago
|
||
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.
Comment 13•8 years ago
|
||
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
Comment 14•8 years ago
|
||
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.
Comment 15•8 years ago
|
||
A few more errors to address, getting there, but don't hold your breath ;-)
Comment 16•8 years ago
|
||
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+
Comment 17•8 years ago
|
||
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
Comment 18•8 years ago
|
||
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 19•8 years ago
|
||
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+
Comment 20•8 years ago
|
||
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.
Comment 21•8 years ago
|
||
Comment 22•8 years ago
|
||
(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).
Comment 23•8 years ago
|
||
(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 24•8 years ago
|
||
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+
Comment 25•8 years ago
|
||
(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
You need to log in
before you can comment on or make changes to this bug.
Description
•