Closed Bug 1328916 Opened 3 years ago Closed 3 years ago

Remove ProfilerSaveSignalHandler and transitively reachable code

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: mstange, Assigned: jseward)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

On B2G the profiler was started from a signal handler. I don't think we need this ability any more.
Blocks: 880165, 1011215
Markus, I'm unclear about this.  On x86_64-linux ProfilerSignalHandler is
definitely used -- I put a printf in to check.

Did you mean ProfilerSaveSignalHandler?
Assignee: nobody → jseward
Flags: needinfo?(mstange)
I can't tell for sure whether ProfilerSaveSignalHandler is used or not, because
it's only reachable by delivery of SIGUSR2, and various other parts of the
system also use SIGUSR2.  Superficially at least though it looks like those
other uses are unrelated.

Assuming that's the case, there's a whole bunch of stuff that can be removed:

remove ProfilerSaveSignalHandler()
-> RequestSave() is never called
   -> RequestSave() can be removed
   -> GeckoSampler::mSaveRequested can never become true
      -> GeckoSampler::HandleSaveRequest is always a no-op
         -> GeckoSampler::HandleSaveRequest can be removed
            -> no instance of class SaveProfileTask is ever made
               -> all of SaveProfileTask and things only reachable from there,
                  can be removed
Attached patch bug1328916-1.cset (obsolete) — Splinter Review
Removes everything mentioned above, up to but not including SaveProfileTask.
Yes, sorry, I meant ProfilerSaveSignalHandler.

I'm pretty sure there aren't any other users of the profiler save signal. It was used by B2G's profile.sh here: https://github.com/mozilla-b2g/B2G/blob/master/profile.sh#L557

I'm curious, why did you choose not to remove SaveProfileTask? I think it should be removed as well.
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #4)
> I'm curious, why did you choose not to remove SaveProfileTask? I think it
> should be removed as well.

Only because I wanted to check I was on the right track before spending
more time on this.  Now that you've confirmed that, I will definitely
remove SaveProfileTask as well.
Summary: Remove ProfilerSignalHandler → Remove ProfilerSaveSignalHandler and transitively reachable code
Removes:
  ProfilerSaveSignalHandler()
  RequestSave()
  GeckoSampler::mSaveRequested
  GeckoSampler::HandleSaveRequest()
  class SaveProfileTask, all of it

This leaves class ProfileSaveEvent stranded in SaveProfileTask.{h,cpp}.
It is not much code and only used in Sampler.cpp.  So I moved it to
to Sampler.cpp and removed the files SaveProfileTask.{h,cpp} entirely.
Attachment #8831696 - Attachment is obsolete: true
Attachment #8832367 - Flags: review?(mstange)
Attachment #8832367 - Flags: review?(mstange) → review+
Comment on attachment 8832367 [details] [diff] [review]
bug1328916-3.cset

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

::: tools/profiler/core/platform-linux.cc
@@ -434,5 @@
> -  if (sigaction(SIGNAL_SAVE_PROFILE, &sa2, &old_sigsave_signal_handler_) != 0) {
> -    LOG("Error installing start signal");
> -    return;
> -  }
> -  LOG("Signal installed");

You might want to leave that last LOG statement in, because it arguably applies to the ProfilerSignalHandler setup above.
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/296fcb87d350
Remove ProfilerSaveSignalHandler and transitively reachable code.  r=mstange.
(In reply to Nicholas Nethercote [:njn] from comment #7)
> You might want to leave that last LOG statement in, [..]

Ah yes, good point.  Fixed.  It was in fact there in an earlier
version of the patch but appears to have been lost during rebasing.
https://hg.mozilla.org/mozilla-central/rev/296fcb87d350
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(In reply to Julian Seward [:jseward] from comment #1)
> Markus, I'm unclear about this.  On x86_64-linux ProfilerSignalHandler is
> definitely used -- I put a printf in to check.
> 
> Did you mean ProfilerSaveSignalHandler?

It turns out that I meant both SigstartHandler and ProfilerSaveSignalHandler. This bug took care of the latter, so I've filed bug 1351946 for the former.
You need to log in before you can comment on or make changes to this bug.