Last Comment Bug 693586 - jprof should support circular buffers
: jprof should support circular buffers
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla10
Assigned To: Randell Jesup [:jesup]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-11 06:44 PDT by Randell Jesup [:jesup]
Modified: 2011-11-08 07:07 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add circular buffer support to jprof (19.49 KB, patch)
2011-10-11 06:46 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Splinter Review
Add circular buffer support to jprof (threadsafe) (20.72 KB, patch)
2011-10-11 07:40 PDT, Randell Jesup [:jesup]
dbaron: review+
Details | Diff | Splinter Review

Description Randell Jesup [:jesup] 2011-10-11 06:44:54 PDT
jprof should support circular buffers, saved on event/signal.  Also it should be possible to clear the buffer on demand.
Comment 1 Randell Jesup [:jesup] 2011-10-11 06:46:12 PDT
Created attachment 566199 [details] [diff] [review]
Add circular buffer support to jprof
Comment 2 Randell Jesup [:jesup] 2011-10-11 06:51:13 PDT
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 ...").
Comment 3 Randell Jesup [:jesup] 2011-10-11 06:52:27 PDT
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.
Comment 4 Randell Jesup [:jesup] 2011-10-11 07:40:31 PDT
Created attachment 566219 [details] [diff] [review]
Add circular buffer support to jprof (threadsafe)
Comment 5 Randell Jesup [:jesup] 2011-11-07 13:29:24 PST
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.
Comment 6 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2011-11-07 13:49:36 PST
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.
Comment 7 Randell Jesup [:jesup] 2011-11-07 18:36:26 PST
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 8 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2011-11-07 20:18:19 PST
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?)
Comment 9 Randell Jesup [:jesup] 2011-11-07 21:14:32 PST
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.
Comment 11 Ed Morley [:emorley] 2011-11-08 07:07:56 PST
https://hg.mozilla.org/mozilla-central/rev/bcf9c9ec97a0

Note You need to log in before you can comment on or make changes to this bug.