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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

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.
More size wins!
Attachment #8893169 - Flags: review?(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.
(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 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.
Attachment #8893169 - Flags: review?(erahm) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb25810c1fb
out-of-line LazyLogModule::operator LogModule*(); r=erahm
https://hg.mozilla.org/mozilla-central/rev/bcb25810c1fb
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

(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.

(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.

Flags: needinfo?(ehsan)

(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.

Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.