Last Comment Bug 686571 - Add preference for turning on JSOPTION_PCCOUNTS
: Add preference for turning on JSOPTION_PCCOUNTS
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Steve Fink [:sfink] [:s:]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-13 15:24 PDT by Steve Fink [:sfink] [:s:]
Modified: 2011-09-20 07:51 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Prevent crash when running code with pccounts on if compiled with pccounts off (1.41 KB, patch)
2011-09-13 15:30 PDT, Steve Fink [:sfink] [:s:]
bhackett1024: review+
Details | Diff | Review
Add preferences for turning on pccounts in the browser (3.62 KB, patch)
2011-09-13 15:30 PDT, Steve Fink [:sfink] [:s:]
dmandelin: review+
Details | Diff | Review
Bug 686571 - Expose new javascript.options.pccounts.(content|chrome) prefs defaults. r=dmandelin (1.21 KB, patch)
2011-09-16 16:57 PDT, Cedric Vivier [:cedricv]
dmandelin: review+
Details | Diff | Review

Description Steve Fink [:sfink] [:s:] 2011-09-13 15:24:27 PDT
I would like a preference to turn on the JSOPTION_PCCOUNTS option (which records execution counts for every Javascript opcode run), so that the collected information can be surfaced through addons.

This is probably the wrong component. This isn't a bug in the preferences system, it's a specific preference.
Comment 1 Steve Fink [:sfink] [:s:] 2011-09-13 15:30:05 PDT
Created attachment 560048 [details] [diff] [review]
Prevent crash when running code with pccounts on if compiled with pccounts off

Need to check whether a pcLengths collection area exists before writing to it (for when pccounts are toggled after compilation.)
Comment 2 Steve Fink [:sfink] [:s:] 2011-09-13 15:30:57 PDT
Created attachment 560049 [details] [diff] [review]
Add preferences for turning on pccounts in the browser
Comment 3 Brian Hackett (:bhackett) 2011-09-13 15:38:11 PDT
Comment on attachment 560048 [details] [diff] [review]
Prevent crash when running code with pccounts on if compiled with pccounts off

If you want to make the JITScript::pcLengths independent from cx->hasRunOption(JSOPTION_PCCOUNT), can you remove the JSOPTION_PCCOUNT test entirely?  It seems like it's pointless now.

Eventually you could make it so that changing the PCCOUNT option's value immediately discards all jitcode in the compartment/runtime, so that future jitcode will reflect the current setting.
Comment 4 David Mandelin [:dmandelin] 2011-09-13 15:50:45 PDT
Comment on attachment 560049 [details] [diff] [review]
Add preferences for turning on pccounts in the browser

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

That options code is starting to get repetitive. It'd be great if someone went in and added some abstraction there.
Comment 5 Steve Fink [:sfink] [:s:] 2011-09-13 17:12:02 PDT
(In reply to Brian Hackett from comment #3)
> Comment on attachment 560048 [details] [diff] [review]
> Prevent crash when running code with pccounts on if compiled with pccounts
> off
> 
> If you want to make the JITScript::pcLengths independent from
> cx->hasRunOption(JSOPTION_PCCOUNT), can you remove the JSOPTION_PCCOUNT test
> entirely?  It seems like it's pointless now.

Well, not entirely. If you compile with JSOPTION_PCCOUNT on, both the interpreter and method JIT will accumulate until you turn it off. With your change, it would continue accumulating and later queries would see counts that include time after you turned the option off.

On the other hand, turning it on won't do anything until a recompile, so you can't usefully bracket periods of time you care about with option on then off anyway. I think your way is better because it's a little simpler.

> Eventually you could make it so that changing the PCCOUNT option's value
> immediately discards all jitcode in the compartment/runtime, so that future
> jitcode will reflect the current setting.

Yes, just like the debugger.
Comment 8 Cedric Vivier [:cedricv] 2011-09-16 16:57:49 PDT
Created attachment 560653 [details] [diff] [review]
Bug 686571 - Expose new javascript.options.pccounts.(content|chrome) prefs defaults. r=dmandelin
Comment 9 Ed Morley [:emorley] 2011-09-20 03:13:13 PDT
Comment on attachment 560653 [details] [diff] [review]
Bug 686571 - Expose new javascript.options.pccounts.(content|chrome) prefs defaults. r=dmandelin

Followup sent to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09b9a8009763

Cedric, to save time for future patches (this one is fine for now), could you set your hgrc to include the author automatically, along the lines of:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Thanks :-)
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 07:51:07 PDT
https://hg.mozilla.org/mozilla-central/rev/09b9a8009763

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