Last Comment Bug 698201 - Remove tracer
: Remove tracer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on: 693815 705380 706611 707341 707351 709792 710481 710486 714894 751280
Blocks: 647628 692838 704356 705168 705356 705401 714052
  Show dependency treegraph
 
Reported: 2011-10-29 08:45 PDT by Tom Schuster [:evilpie]
Modified: 2012-05-02 11:46 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: remove major files (2.27 MB, patch)
2011-11-09 13:53 PST, David Anderson [:dvander]
dmandelin: review+
Details | Diff | Splinter Review
part 2: remove imacros (38.10 KB, patch)
2011-11-09 13:54 PST, David Anderson [:dvander]
luke: review+
Details | Diff | Splinter Review
part 3: remove traceable natives from js/src (97.57 KB, patch)
2011-11-09 13:55 PST, David Anderson [:dvander]
luke: review+
Details | Diff | Splinter Review
part 4: remove everything def'd under JS_TRACER (55.43 KB, patch)
2011-11-09 13:56 PST, David Anderson [:dvander]
bhackett1024: review+
Details | Diff | Splinter Review
part 5: remove structures, functions (88.27 KB, patch)
2011-11-09 13:57 PST, David Anderson [:dvander]
luke: review+
Details | Diff | Splinter Review
part 6: xpconnect changes (42.80 KB, patch)
2011-11-09 14:00 PST, David Anderson [:dvander]
bobbyholley: review+
Details | Diff | Splinter Review
part 7: remove browser prefs (7.52 KB, patch)
2011-11-09 14:02 PST, David Anderson [:dvander]
gal: review+
gal: superreview+
Details | Diff | Splinter Review
Remove some left-overs from JM (20.96 KB, patch)
2011-11-23 08:38 PST, Tom Schuster [:evilpie]
dvander: review+
Details | Diff | Splinter Review

Description Tom Schuster [:evilpie] 2011-10-29 08:45:55 PDT
The time has come.

tom@tom-linux:~/inbound/js/src$ hg log jstracer.cpp | grep -i "bug" -c
1474

Brain thinks dvander has a patch for this?
Comment 1 Cameron Kaiser [:spectre] 2011-10-30 08:21:03 PDT
Wait, I thought this wasn't happening until 11?
Comment 2 Tom Schuster [:evilpie] 2011-10-30 08:26:49 PDT
This is just for bookkeeping. It is supposed to be done after FF10 merges to Aurora.
Comment 3 Nicholas Nethercote [:njn] 2011-10-30 20:34:26 PDT
Note: don't forget the tracer-related stuff in xpconnect/src/XPCJSRuntime.cpp.
Comment 4 David Anderson [:dvander] 2011-11-09 13:53:17 PST
Created attachment 573310 [details] [diff] [review]
part 1: remove major files

Remove nanojit/*, jstracer*, lirasm/*, and friends.

This queue is based on top of Bill's write barriers patch. I'll send it to try soon - the browser builds locally and tests pass.
Comment 5 David Anderson [:dvander] 2011-11-09 13:54:01 PST
Created attachment 573311 [details] [diff] [review]
part 2: remove imacros
Comment 6 David Anderson [:dvander] 2011-11-09 13:55:50 PST
Created attachment 573313 [details] [diff] [review]
part 3: remove traceable natives from js/src
Comment 7 David Anderson [:dvander] 2011-11-09 13:56:49 PST
Created attachment 573314 [details] [diff] [review]
part 4: remove everything def'd under JS_TRACER
Comment 8 David Anderson [:dvander] 2011-11-09 13:57:58 PST
Created attachment 573315 [details] [diff] [review]
part 5: remove structures, functions
Comment 9 David Anderson [:dvander] 2011-11-09 14:00:14 PST
Created attachment 573316 [details] [diff] [review]
part 6: xpconnect changes
Comment 10 David Anderson [:dvander] 2011-11-09 14:02:52 PST
Created attachment 573317 [details] [diff] [review]
part 7: remove browser prefs
Comment 11 Luke Wagner [:luke] 2011-11-09 14:36:08 PST
Comment on attachment 573311 [details] [diff] [review]
part 2: remove imacros

r++++++ would review again
Comment 12 Luke Wagner [:luke] 2011-11-09 14:43:38 PST
Comment on attachment 573315 [details] [diff] [review]
part 5: remove structures, functions

I'd be inclined to just remove JSOPTION_JIT.
Comment 13 Brian Hackett (:bhackett) 2011-11-09 14:44:38 PST
Comment on attachment 573314 [details] [diff] [review]
part 4: remove everything def'd under JS_TRACER

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

What is the plan for the deprecated API bits like JSOPTION_JIT and JS_SetGCParameterForThread (and no-ops below it)?
Comment 14 Nicholas Nethercote [:njn] 2011-11-09 14:57:13 PST
Can you run "diffstats" on the combined patches? :)
Comment 15 Andreas Gal :gal 2011-11-09 15:18:11 PST
Comment on attachment 573317 [details] [diff] [review]
part 7: remove browser prefs

r=me, the tracer shall rise again, if it makes sense down the road
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2011-11-09 15:55:33 PST
Comment on attachment 573316 [details] [diff] [review]
part 6: xpconnect changes

\o/
Comment 17 David Anderson [:dvander] 2011-11-09 19:41:19 PST
(In reply to Brian Hackett from comment #13)
> What is the plan for the deprecated API bits like JSOPTION_JIT and
> JS_SetGCParameterForThread (and no-ops below it)?

I'll get rid of JSOPTION_JIT. Do you think I should get rid of the other ones as well? It doesn't seem like we have plans for them.

(In reply to Nicholas Nethercote [:njn] from comment #14)
> Can you run "diffstats" on the combined patches? :)

 350 files changed, 126 insertions(+), 67643 deletions(-)
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2011-11-14 04:07:28 PST
Ship it, I say!
Comment 19 :Ms2ger (⌚ UTC+1/+2) 2011-11-15 02:41:36 PST
... Once it passes try.

https://tbpl.mozilla.org/?tree=Try&rev=7e79a6ec0b0c

doesn't look good.
Comment 20 Nicholas Nethercote [:njn] 2011-11-15 14:15:51 PST
Actually, since the tracer was already turned off, removing it from the codebase won't make any difference to memory usage.  So I've removed the MemShrink tag.
Comment 21 Nicholas Nethercote [:njn] 2011-11-15 14:16:40 PST
And by "turned off" I mean "not built by default".
Comment 24 Tom Schuster [:evilpie] 2011-11-23 06:22:29 PST
\o/

Why is "jstracer.cpp" still there?
Comment 25 Andrew McCreight [:mccr8] 2011-11-23 06:36:12 PST
jandem pointed out in IRC that the removal of jstracer.cpp is present in the reviewed patch attached to the bug, but not in the patch landed on inbound.
Comment 26 David Anderson [:dvander] 2011-11-23 06:59:38 PST
Thanks, I'm not sure how that happened but I pushed a follow-up fix:

http://hg.mozilla.org/integration/mozilla-inbound/rev/4864134dad4e
Comment 27 Tom Schuster [:evilpie] 2011-11-23 08:38:07 PST
Created attachment 576516 [details] [diff] [review]
Remove some left-overs from JM
Comment 28 David Anderson [:dvander] 2011-11-23 09:03:37 PST
Comment on attachment 576516 [details] [diff] [review]
Remove some left-overs from JM

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

Looks good, but let's do follow-up bugs for future tracer-ectomies.

::: js/src/methodjit/MethodJIT-inl.h
@@ +90,5 @@
>      JITScriptStatus status = script->getJITStatus(fp->isConstructing());
>      if (status == JITScript_Invalid)
>          return Compile_Abort;
>      if (status == JITScript_None && !cx->hasRunOption(JSOPTION_METHODJIT_ALWAYS)) {
> +        if ((cx->typeInferenceEnabled())

Nit: extra parens
Comment 29 Matt Brubeck (:mbrubeck) 2011-11-23 10:13:27 PST
Follow-up to remove jstracer.cpp merged to trunk:
https://hg.mozilla.org/mozilla-central/rev/4864134dad4e
Comment 30 Zio_Fede 2011-11-24 22:34:54 PST
There are still the javascript.options.jitprofiling about:config voices. Shouldn't they be removed?
Comment 31 :Ms2ger (⌚ UTC+1/+2) 2011-11-25 00:54:58 PST
(In reply to Zio_Fede from comment #30)
> There are still the javascript.options.jitprofiling about:config voices.
> Shouldn't they be removed?

Followup, please. (But doesn't that apply to other jits as well?)
Comment 32 Ryan VanderMeulen [:RyanVM] 2011-11-25 14:49:32 PST
My patch in bug 705356 removes the profiling options as well. Let's use that bug for that discussion.

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