Closed Bug 1258436 Opened 4 years ago Closed 4 years ago

Remove GC suppression in JSFunction::createScriptForLazilyInterpretedFunction

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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 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)
It works great with annotation as suggested.
Attachment #8733343 - Attachment is obsolete: true
Attachment #8733790 - Flags: review?(sphink)
Attachment #8733790 - Flags: review?(sphink) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f66ab359f3ef
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1266434
You need to log in before you can comment on or make changes to this bug.