1386918 - out-of-line LazyLogModule::operator LogModule*()

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nathan Froyd [:froydnj]]

This function takes up quite a bit of space, and there's no need to for
the log getter to be inlined everywhere.  Moving this to out-of-line
code saves ~200K on x86-64 Linux.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: out-of-line LazyLogModule::operator LogModule*()

More size wins!

Comment 2

[Eric Rahm [:erahm]]

That's a big savings! I'm surprised we didn't get push back when it landed. Does this have a perf impact? One of the key things for allowing always available logging was that it didn't cause perf regressions for release builds.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to (out Aug 3 - 6) Eric Rahm [:erahm] (please no mozreview requests) from comment #2)
> That's a big savings! I'm surprised we didn't get push back when it landed.
> Does this have a perf impact? One of the key things for allowing always
> available logging was that it didn't cause perf regressions for release
> builds.

I haven't measured the perf impact, but I can do that.

I guess it didn't cause perf impact when it initially landed because before, we had a sequence like:

  load global variable

and now we have:

  load member variable
  well-predicted branch

which is roughly the same.  Also, I think we converted PR_LogModule to LazyLogModule in stages, so individual changes may not have been significant, even if they might have been in aggregate?

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8893169 [details] [diff] [review]
out-of-line LazyLogModule::operator LogModule*()

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

Lets see how this goes; Talos or the quantum flow people will let us know if there's a perf hit.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb25810c1fb
out-of-line LazyLogModule::operator LogModule*(); r=erahm

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/bcb25810c1fb

Comment 7

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #3)

(In reply to (out Aug 3 - 6) Eric Rahm [:erahm] (please no mozreview
requests) from comment #2)

That's a big savings! I'm surprised we didn't get push back when it landed.
Does this have a perf impact? One of the key things for allowing always
available logging was that it didn't cause perf regressions for release
builds.

I haven't measured the perf impact, but I can do that.

When looking at profiles of our AntiTracking code which logs heavily (for bug 1516540), I see this non-inline function show up quite a lot. :-( In some functions it's around 10% of self-time.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari from comment #7)

(In reply to Nathan Froyd [:froydnj] from comment #3)

(In reply to (out Aug 3 - 6) Eric Rahm [:erahm] (please no mozreview
requests) from comment #2)

That's a big savings! I'm surprised we didn't get push back when it landed.
Does this have a perf impact? One of the key things for allowing always
available logging was that it didn't cause perf regressions for release
builds.

I haven't measured the perf impact, but I can do that.

When looking at profiles of our AntiTracking code which logs heavily (for bug 1516540), I see this non-inline function show up quite a lot. :-( In some functions it's around 10% of self-time.

This shows up even when you're not logging? That's...not good. Do you think that's the fault of the log code itself, or just an excessive amount of logging from the AntiTracking code?

Please file a separate bug for that if you think there's something we could do here.

Comment 9

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #8)

(In reply to :Ehsan Akhgari from comment #7)

(In reply to Nathan Froyd [:froydnj] from comment #3)

(In reply to (out Aug 3 - 6) Eric Rahm [:erahm] (please no mozreview
requests) from comment #2)

That's a big savings! I'm surprised we didn't get push back when it landed.
Does this have a perf impact? One of the key things for allowing always
available logging was that it didn't cause perf regressions for release
builds.

I haven't measured the perf impact, but I can do that.

When looking at profiles of our AntiTracking code which logs heavily (for bug 1516540), I see this non-inline function show up quite a lot. :-( In some functions it's around 10% of self-time.

This shows up even when you're not logging?

Yes, when not logging. :(

That's...not good. Do you think that's the fault of the log code itself, or just an excessive amount of logging from the AntiTracking code?

Neither, it's the fault of this patch IMO. According to perf, all of the overhead is just there due to the non-inline call.

(FWIW this is the function I was looking at: https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/toolkit/components/antitracking/AntiTrackingCommon.cpp#1324. In the non-error cases, it will call LazyLogModule::operator LogModule*() exactly twice, once on line 1329 and once on line 1391. I don't think that's excessive logging, but please double check my assumption here.)

Please file a separate bug for that if you think there's something we could do here.

Filed bug 1525303.