jprof should support circular buffers

RESOLVED FIXED in mozilla10

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

Trunk
mozilla10
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
jprof should support circular buffers, saved on event/signal.  Also it should be possible to clear the buffer on demand.
(Assignee)

Comment 1

6 years ago
Created attachment 566199 [details] [diff] [review]
Add circular buffer support to jprof
(Assignee)

Comment 2

6 years ago
This includes JS functions:

JProfClearCircular() and JProfSaveCircular()

These are handy for stuff like:

top_of_event_loop:
     JProfClearCircular();
     process_events();
     if (processing took too long)
        JProfSaveCircular();

There are other uses as well, like running a continuous circular buffer and saving it when you notice a problem, or clearing it right before repeating an action and saving right after (though that usecase can also be addressed by having a jprof-enabled build and using kill -PROF right before, and kill -USR1 right after, and analyze with "jprof --last ...").
(Assignee)

Comment 3

6 years ago
Note: I haven't scrubbed the patch for style; and the style of jprof tends to be at odds with current source style a bit - I stick to the existing style of the file.
(Assignee)

Comment 4

6 years ago
Created attachment 566219 [details] [diff] [review]
Add circular buffer support to jprof (threadsafe)
(Assignee)

Updated

6 years ago
Attachment #566199 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
Comment on attachment 566219 [details] [diff] [review]
Add circular buffer support to jprof (threadsafe)

Asking for rs+ for FF10; I'll roll any improvements into 11 (unless I can get them in and reviewed tonight).  NPOTB by default...

I'll turn off the printf's in NS_JProfStartProfiling() before checking it in.
Attachment #566219 - Flags: review?(dbaron)
What's threadsafe about this, and why does it need to be threadsafe?

Should the indentation in the definition of DumbCircularBuffer match one of the two existing indentation styles in the rest of the file rather than adding a third?

I'm inclined to rubber-stamp this if I understand the answer to the first question, unless there's something in particular you want me to look at.
(Assignee)

Comment 7

6 years ago
The DumbCircularBuffer stuff uses a mutex to guarantee safety since we now have a shared structure between threads with pointers.  You can take a profile timer interrupt in any thread, and while you won't get interrupts in multiple threads, you may get another thread clearing the circular buffer or saving it while the interrupt is trying to update the buffer, so some form of locking is needed, or to push off those operations to occur within the interrupt (using flags).  I also have a plan to make other uses of it that may require inter-thread locking, but that can be worried about later.

I added the lock at glandium's suggestion in IRC, if I remember.  I was doing this quickly and wanted to ensure a functional solution; a lower-overhead solution using deferred clear or save may be done in a cleanup.

I can correct the indentation style before checkin.
Comment on attachment 566219 [details] [diff] [review]
Add circular buffer support to jprof (threadsafe)

ok, r=dbaron

(Do you know what the rules for which thread a signal handler gets invoked on are?  Can other threads ever continue running while a signal handler runs?)
Attachment #566219 - Flags: review?(dbaron) → review+
(Assignee)

Comment 9

6 years ago
On linux, a process-level signal can be delivered to any thread that doesn't have the signal blocked.  In practice, I think it's normally the executing or most-recent-executing thread, and jprof pretty much depends on this.  Other threads don't run while a signal handler is running in one, so far as I know.
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf9c9ec97a0
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/bcf9c9ec97a0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.