947400 - Make the analysis consider whether a virtual call can GC when all targets are known

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Hazard:

Function 'js::ScriptSourceObject* js::ScriptSourceObject::create(js::ExclusiveContext*, js::ScriptSource*, JS::ReadOnlyCompileOptions*)' has unrooted '__temp_11' of type 'js::ScriptSourceObject*' live across GC call JS::ReadOnlyCompileOptions.element at js/src/jsscript.cpp:1009
    js/src/jsscript.cpp:1009: Call(21,22, __temp_13 := options*.element*())
    js/src/jsscript.cpp:1009: Call(22,23, __temp_12 := ObjectOrNullValue(__temp_13*))
    js/src/jsscript.cpp:1009: Call(23,24, __temp_11*.field:0.field:0.initSlot(1,__temp_12))

The problem is that ReadOnlyCompileOptions::element() is a virtual function call. The implementations are trivial and certainly cannot GC, and the analysis knows this when constructing the callgraph. But when looking for hazards, the callgraph is not involved for FieldCalls, so it assumes that any non-annotated FieldCall can gc.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Make the analysis consider whether a virtual call can GC when all targets are known

When determining all targets of a virtual function call, record those targets in callgraph.txt. Then when computing gcFunctions, check whether any of them can GC and if not, add the FieldCall into suppressedFunctions. It turns out that suppressedFunctions *already* contained field calls, so this turns out to be straightforward. Note that any virtual function call that cannot be resolved to a known set of target callees will remain the same as it is now, and so will be assumed to GC.

This brings the current shell hazard count down to zero (!).

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 8343940 [details] [diff] [review]
Make the analysis consider whether a virtual call can GC when all targets are known

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

::: js/src/devtools/rootAnalysis/analyzeRoots.js
@@ +237,5 @@
>          var csuName = field.FieldCSU.Type.Name;
>          var fullFieldName = csuName + "." + field.Name[0];
>          if (fieldCallCannotGC(csuName, fullFieldName))
>              return null;
> +        //printErr(fullFieldName + ": " + ((fullFieldName in suppressedFunctions) ? "suppressed" : "cangc"));

rm debugging code

::: js/src/devtools/rootAnalysis/computeCallgraph.js
@@ +151,5 @@
> +                // calls to all possible resolved types, but also record edges
> +                // from this field call to each final callee. When the analysis
> +                // is checking whether an edge can GC and it sees an unrooted
> +                // pointer held live across this field call, it will know
> +                // whether any of the direct callers can GC or not.

s/callers/callees

::: js/src/vm/StructuredClone.cpp
@@ +395,5 @@
> +        if (tag == SCTAG_TRANSFER_MAP_ARRAY_BUFFER) {
> +            if (ownership == JS::SCTAG_TMO_ALLOC_DATA)
> +                js_free(content);
> +        } else if (cb && cb->freeTransfer) {
> +            cb->freeTransfer(buffer, tag, JS::TransferableOwnership(ownership), content, cbClosure);

This change looks unrelated?

Comment 3

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a55439a67e05
http://hg.mozilla.org/integration/mozilla-inbound/rev/5c34885094e4

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c34885094e4
https://hg.mozilla.org/mozilla-central/rev/a55439a67e05