1162336 - Remove usage of |#ifdef PR_LOGGING| from netwerk

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Eric Rahm [:erahm]]

In bug 1161238 we plan on removing |--disable-logging| which makes |#ifdef PR_LOGGING| redundant.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Remove instances of #ifdef PR_LOGGING in netwerk

PR_LOGGING is now always defined, we can remove #ifdefs checking for it.

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Wrap expensive calls in PR_LOG_TEST

Check that logging is enabled before performing potentially expensive
operations.

Comment 3

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e249ee55a442

Comment 4

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be9b7577631b

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8602440 [details] [diff] [review]
Part 1: Remove instances of #ifdef PR_LOGGING in netwerk

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

::: netwerk/base/nsPACMan.cpp
@@ -25,5 @@
>  using namespace mozilla;
>  using namespace mozilla::net;
>  
> -#if defined(PR_LOGGING)
> -#endif

:)

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8602441 [details] [diff] [review]
Part 2: Wrap expensive calls in PR_LOG_TEST

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

Honza, please see the comments for the particular feedback requested here.

::: netwerk/cache2/CacheFileContextEvictor.cpp
@@ +248,5 @@
>  
>    nsAutoCString path;
> +  if (LOG_ENABLED()) {
> +    file->GetNativePath(path);
> +  }

I tend to think moving this block down into the NS_WARN_IF block:

if (NS_WARN_IF(...)) {
  if (LOG_ENABLED()) {
    nsAutoCString path;
    ...
    LOG((...));
  }
}

is better, but I can also see that one might want to break on the OpenNSPRFileDesc call conditionally on the value of path.  So I'm going to ask Honza about this one...

@@ +287,5 @@
>  
>    nsAutoCString path;
> +  if (LOG_ENABLED()) {
> +    file->GetNativePath(path);
> +  }

...and this one, since this is code he's familiar with.

Comment 7

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #6)
> Comment on attachment 8602441 [details] [diff] [review]
> Part 2: Wrap expensive calls in PR_LOG_TEST
> 
> Review of attachment 8602441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Honza, please see the comments for the particular feedback requested here.
> 
> ::: netwerk/cache2/CacheFileContextEvictor.cpp
> @@ +248,5 @@
> >  
> >    nsAutoCString path;
> > +  if (LOG_ENABLED()) {
> > +    file->GetNativePath(path);
> > +  }
> 
> I tend to think moving this block down into the NS_WARN_IF block:
> 
> if (NS_WARN_IF(...)) {
>   if (LOG_ENABLED()) {
>     nsAutoCString path;
>     ...
>     LOG((...));
>   }
> }
> 
> is better, but I can also see that one might want to break on the
> OpenNSPRFileDesc call conditionally on the value of path.  So I'm going to
> ask Honza about this one...
> 

I originally did that, but it broke the build. |path| is used again further down in the function (which of course is not obvious as we don't have expandable context in splinter).


> @@ +287,5 @@
> >  
> >    nsAutoCString path;
> > +  if (LOG_ENABLED()) {
> > +    file->GetNativePath(path);
> > +  }
> 
> ...and this one, since this is code he's familiar with.

Same deal.

Comment 8

[Nathan Froyd [:froydnj]]

Doh, I had meant to look at the file, but apparently forgot.  OK, these are fine, then.

Comment 9

[Nathan Froyd [:froydnj]]

Actually, scratch that.  Since we've had logging globally enabled for some time, this doesn't seem to be a problem, so let's forgo the conditionally-initialized variables in this case.  The other cases are straightforward code, so let's leave them as-is.

Comment 10

[Eric Rahm [:erahm]]

Will remove those two changes before I land.

Comment 11

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0170a6c2a5be
https://hg.mozilla.org/integration/mozilla-inbound/rev/8952a7fa4050

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Both commits backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a74ede09893f for breaking at least Windows builds:

https://treeherder.mozilla.org/logviewer.html#?job_id=9657250&repo=mozilla-inbound

Comment 13

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01f2a7e1a2bd

Comment 14

[Eric Rahm [:erahm]]

It would appear netwerk/system/win32/nsNotifyAddrListener.cpp was not including prlog.h, pushed a few windows try builds to verify that's it.

Comment 15

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0313bc7f6052
https://hg.mozilla.org/integration/mozilla-inbound/rev/f326f7d6ff52

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0313bc7f6052
https://hg.mozilla.org/mozilla-central/rev/f326f7d6ff52