Closed
Bug 1258436
Opened 8 years ago
Closed 8 years ago
Remove GC suppression in JSFunction::createScriptForLazilyInterpretedFunction
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(1 file, 1 obsolete file)
3.13 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
There's an AutoSuppressGC in this method with an out-of-date comment that says we need to do this "for now". We should be able to just remove it.
Assignee | ||
Comment 1•8 years ago
|
||
It was slightly more complicated than just removing the suppression, due to a couple of hazards that showed up (one real, one not).
Attachment #8733343 -
Flags: review?(sphink)
Comment 2•8 years ago
|
||
Comment on attachment 8733343 [details] [diff] [review] bug1258436-lazy-script-suppression Review of attachment 8733343 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscriptinlines.h @@ +87,5 @@ > inline JSFunction* > LazyScript::functionDelazifying(JSContext* cx) const > { > + Rooted<const LazyScript*> self(cx, this); > + if (self->function_ && !self->function_->getOrCreateScript(cx)) Ugh. I was going to say I'd rather have this static, but after looking closer it seems like it's a pretty big mess what with the zero-arg functionDelazifying(). I guess this is ok. ::: js/src/vm/Compression.cpp @@ +114,5 @@ > #endif > > + // The hazard analysis thinks this can GC due to a field calls in inflate. > + // It can't. > + JS::AutoSuppressGCAnalysis nogc; I would definitely prefer to annotate this one. I *think* all that should be needed is: diff --git a/js/src/devtools/rootAnalysis/annotations.js b/js/src/devtools/rootAnalysis/annotations.js --- a/js/src/devtools/rootAnalysis/annotations.js +++ b/js/src/devtools/rootAnalysis/annotations.js @@ -71,16 +71,17 @@ var ignoreCallees = { "js::Class.trace" : true, "js::Class.finalize" : true, "JSRuntime.destroyPrincipals" : true, "icu_50::UObject.__deleting_dtor" : true, // destructors in ICU code can't cause GC "mozilla::CycleCollectedJSRuntime.DescribeCustomObjects" : true, // During tracing, cannot GC. "mozilla::CycleCollectedJSRuntime.NoteCustomGCThingXPCOMChildren" : true, // During tracing, cannot GC. "PLDHashTableOps.hashKey" : true, "z_stream_s.zfree" : true, + "z_stream_s.zalloc" : true, "GrGLInterface.fCallback" : true, "std::strstreambuf._M_alloc_fun" : true, "std::strstreambuf._M_free_fun" : true,
Attachment #8733343 -
Flags: review?(sphink)
Assignee | ||
Comment 3•8 years ago
|
||
It works great with annotation as suggested.
Attachment #8733343 -
Attachment is obsolete: true
Attachment #8733790 -
Flags: review?(sphink)
Updated•8 years ago
|
Attachment #8733790 -
Flags: review?(sphink) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 5•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f66ab359f3ef
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•