Closed Bug 903212 Opened 6 years ago Closed 6 years ago

Remove nsIScriptContext::ScriptEvaluated

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files, 1 obsolete file)

Once bug 901364 lands and the slow script dialog machinery is no longer tangled up in nsJSContext, ScriptEvaluted can more or less go away, which is awesome. It does a few more vestigial things, but those can be handled relatively easily, I think.
Blocks: 901106
Assuming that try-opt builds and tinderbox m-c builds are comparable, this does not affect sunspider times:

Baseline: http://bit.ly/13XtFwT
Patches: http://bit.ly/1czFmQ6
This only has an impact when we enable explict compartmental GCs, which have
been preffed off for a while.
Attachment #790005 - Flags: review?(bugs)
I think that this is more or less isomorphic with the MaybeGC we do in
ScriptEvaluated in the cases that matter. And doing it in a spec-defined
place is helpful in getting rid of spec-nonsensical machinery.
Attachment #790006 - Flags: review?(mrbkap)
Comment on attachment 790005 [details] [diff] [review]
Part 1 - Remove activity tracking from nsJSContext. v1

You should remove sDisableExplicitCompartmentGC everywhere, and the
pref controlling it from all.js (the pref has been true over a year)
javascript.options.mem.disable_explicit_compartment_gc
And remove NS_MAX_COMPARTMENT_GC_COUNT too.
And sCompartmentGCCount
And I think sContextList too.
Attachment #790005 - Flags: review?(bugs) → review-
Comment on attachment 790005 [details] [diff] [review]
Part 1 - Remove activity tracking from nsJSContext. v1

(In reply to Olli Pettay [:smaug] from comment #7)
> You should remove sDisableExplicitCompartmentGC everywhere, and the
> pref controlling it from all.js (the pref has been true over a year)
> javascript.options.mem.disable_explicit_compartment_gc
> And remove NS_MAX_COMPARTMENT_GC_COUNT too.
> And sCompartmentGCCount
> And I think sContextList too.

Can I do that in a followup patch please?
Attachment #790005 - Flags: review- → review?(bugs)
Attached patch part 1, remove pref etc (obsolete) — Splinter Review
I'd prefer not leaving trivial stuff to followups.
Here is the patch which removes the pref and various variables.
Attachment #790005 - Attachment is obsolete: true
Attachment #790005 - Flags: review?(bugs)
Attachment #790324 - Flags: review?(bobbyholley+bmo)
I need the context list stuff for my patches in bug 899367.
Attachment #790324 - Attachment is obsolete: true
Attachment #790324 - Flags: review?(bobbyholley+bmo)
Attachment #790348 - Flags: review?(bugs)
Attachment #790005 - Flags: review?(bugs)
Attachment #790005 - Attachment is obsolete: false
Attachment #790006 - Flags: review?(mrbkap) → review+
Attachment #790348 - Flags: review?(bugs) → review+
Attachment #790005 - Flags: review?(bugs) → review+
Attachment #790007 - Flags: review?(mrbkap) → review+
Green try push in comment 1, but I added an extra patch that smaug asked for, so doing a quick linux64 try push just to be safe:

https://tbpl.mozilla.org/?tree=Try&rev=f638d36d5a74
Attachment #790008 - Flags: review?(mrbkap) → review+
Duplicate of this bug: 807369
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.