Closed Bug 777548 Opened 8 years ago Closed 7 years ago

GC: Make non-ccParticipant browser object tracing indirect

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: terrence, Assigned: terrence)

References

Details

(Whiteboard: [js:t])

Attachments

(4 files, 1 obsolete file)

This is the easy part: it doesn't touch nsCycleCollectorParticipant.
Whiteboard: [js:t]
Attached patch v0 (obsolete) — Splinter Review
Try run is up at https://tbpl.mozilla.org/?tree=Try&rev=3062df05a34b and appears nicely verdant so far.

These changes are all over the browser, but are generally small, self contained, and very much tracing specific.  Let me know if you want a different reviewer for any of the bits.
Attachment #647675 - Flags: review?(wmccloskey)
Comment on attachment 647675 [details] [diff] [review]
v0

You should at minimum get a DOM person to look at the browser stuff, like Olli maybe.
Attachment #647675 - Flags: review?(bugs)
Depends on: 779336
Comment on attachment 647675 [details] [diff] [review]
v0


>+  void TraceGlobalJSObject(JSTracer *trc)
s/JSTracer *trc/JSTracer* aTrc/

>+  {
>+    if (mJSObject)
>+      JS_CallObjectTracer(trc, &mJSObject, "active window global");
if (expr) {
  stmt;
}

> 
>+  void *GetJSObjectLocation() const
* goes with void

>+  {
>+    return (void *)&mWrapperPtrBits;
C++ casting please.

>+++ b/dom/bindings/BindingUtils.h
>@@ -222,20 +222,18 @@ inline void
> TraceProtoOrIfaceCache(JSTracer* trc, JSObject* obj)
> {
>   MOZ_ASSERT(js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL);
> 
>   if (!HasProtoOrIfaceArray(obj))
>     return;
>   JSObject** protoOrIfaceArray = GetProtoOrIfaceArray(obj);
>   for (size_t i = 0; i < kProtoOrIfaceCacheCount; ++i) {
>-    JSObject* proto = protoOrIfaceArray[i];
>-    if (proto) {
>-      JS_CALL_OBJECT_TRACER(trc, proto, "protoOrIfaceArray[i]");
>-    }
>+    if (protoOrIfaceArray[i])
>+      JS_CallObjectTracer(trc, &protoOrIfaceArray[i], "protoOrIfaceArray[i]");
if (expr) {
  stmt;
}


>   // Our NPIdentifier is not always interned, so we must root it explicitly.
>   jsid id = NPIdentifierToJSId(memberPrivate->methodName);
>-  if (JSID_IS_STRING(id))
>-    JS_CALL_STRING_TRACER(trc, JSID_TO_STRING(id), "NPObjectMemberPrivate.methodName");
>+  if (JSID_IS_STRING(id)) {
>+    JS_SET_TRACING_LOCATION(trc, (void *)&memberPrivate->methodName);
C++ casting, please


>+    JS_CallIdTracer(trc, &id, "NPObjectMemberPrivate.methodName");
>+    memberPrivate->methodName = JSIdToNPIdentifier(id);
>+  }
I don't understand this change. Needs some comment.



> DOMBindingBase::_trace(JSTracer* aTrc)
> {
>-  JSObject* obj = GetJSObject();
>+  JSObject* obj = GetJSObjectFromBits();
>   if (obj) {
>-    JS_CALL_OBJECT_TRACER(aTrc, obj, "cached wrapper");
>+    JS_SET_TRACING_LOCATION(aTrc, GetJSObjectLocation());
>+    JS_CallObjectTracer(aTrc, &obj, "cached wrapper");
>+    SetWrapperBits(obj);
Same thing here. Does JS_SET_TRACING_LOCATION modify the latter parameter.
Manually setting the value back looks pretty error prone.
Would it be possible to use some stack object to set back the value?


> XMLHttpRequest::_trace(JSTracer* aTrc)
> {
>   if (mUpload) {
>-    JS_CALL_OBJECT_TRACER(aTrc, mUpload->GetJSObject(), "mUpload");
>+    JS_SET_TRACING_LOCATION(aTrc, mUpload->GetJSObjectLocation());
>+    JSObject *obj = mUpload->GetJSObject();
>+    JS_CallObjectTracer(aTrc, &obj, "mUpload");
>+    mUpload->SetJSObject(obj);
>   }
And here...

>     FieldInfoHash* fields =
>       static_cast<FieldInfoHash*>(JSVAL_TO_PRIVATE(slot));
>-    for (FieldInfoHash::Range r = fields->all(); !r.empty(); r.popFront()) {
>-      JS_CALL_TRACER(trc, r.front().key, JSTRACE_STRING, "fieldName");
>-      JS_CALL_TRACER(trc, r.front().value.mType, JSTRACE_OBJECT, "fieldType");
>+    for (FieldInfoHash::Enum e(*fields); !e.empty(); e.popFront()) {
>+      JS_CallObjectTracer(trc, &e.front().value.mType, "fieldType");
>+      JSFlatString *key = e.front().key;
* goes with the type
Attachment #647675 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #3)

Thanks for the fast review and the patient style feedback!

> > DOMBindingBase::_trace(JSTracer* aTrc)
> > {
> >-  JSObject* obj = GetJSObject();
> >+  JSObject* obj = GetJSObjectFromBits();
> >   if (obj) {
> >-    JS_CALL_OBJECT_TRACER(aTrc, obj, "cached wrapper");
> >+    JS_SET_TRACING_LOCATION(aTrc, GetJSObjectLocation());
> >+    JS_CallObjectTracer(aTrc, &obj, "cached wrapper");
> >+    SetWrapperBits(obj);
> Same thing here. Does JS_SET_TRACING_LOCATION modify the latter parameter.
> Manually setting the value back looks pretty error prone.
> Would it be possible to use some stack object to set back the value?

The key change we are making in this patch is to update the browser trace functions to take the location of the traced thing rather than the thing itself.  This is so that the GC can update the reference to the thing when it moves objects during compacting GC, generational GC, etc.

In cases where the object pointer is, for example, stored as a tagged pointer, we cannot update the pointer in-place: the GC doesn't know about the tags.  In these cases, we have to manually pull out an untagged version that the GC can understand, update it on the stack, then put it back where it came from with the tags intact.

JS_SET_TRACING_LOCATION is an annotation for the tracer that tells it the real location of the object that is being marked.  We need this to correlate runtime updates to the object graph (that only know the location of the updated thing) with the location that we actually trace here.  I agree that this name is not terribly clear: it is mostly that way to match the JS_SET_TRACING_NAME, JS_SET_TRACING_INDEX, etc.

An RAII guard would doubtlessly be less error-prone, but the 3 cases I found here appear to be one-offs.  Is it really worth the extra code?
Could we have a method which takes the location of the object and a mask?
Something like JS_CallMaskedObjectTracer(trc, GetJSObjectLocation(), kWrapperBitMask);
Then JS eng could do its magic, and use the mask 1) to get the right address for the object and 2)
to update the value.

(I'm hoping to have as simple API as possible, so that not all tracer code need to be reviewed by
js eng hackers)
Depends on: 855145
It may be worthwhile to read the new documentation above the JS_CallTrace family of functions (in patch 1 of 2): it directly addresses many of your questions from the previous iteration.

I think this version came out much better. Of particular note is that it tries to push the call to the trace function down to the level that has direct access to the actual bits, rather than trying to pull the addresses up.
Attachment #647675 - Attachment is obsolete: true
Attachment #731376 - Flags: review?(bugs)
Comment on attachment 731376 [details] [diff] [review]
part 2 of 2: v1 - Rewritten from scratch.

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

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +401,5 @@
> +                JSObject *updated = e.front();
> +                JS_SET_TRACING_LOCATION(trc, static_cast<void *>(&e.front()));
> +                JS_CallObjectTracer(trc, &updated, "DOM expando object");
> +                if (updated != e.front())
> +                    e.rekeyFront(updated);

Note to self: Bobby (and basic politeness) would like a comment here explaining why this needs to be so complicated.
There needs to be a comment in the source code why that is so complicated.
This should be simpler from the XPConnect side, but I worry that it is a bit overly-specific for API. Bill, Olli, does this look like a good idea? Is there a better way to do the same thing?
Attachment #731989 - Flags: feedback?(wmccloskey)
Attachment #731989 - Flags: feedback?(bugs)
Comment on attachment 731989 [details] [diff] [review]
Move HashSet marker internal.

Well, looks better from API user's point of view, IMO.
Attachment #731989 - Flags: feedback?(bugs) → feedback+
Comment on attachment 731376 [details] [diff] [review]
part 2 of 2: v1 - Rewritten from scratch.


>+inline void
>+nsWrapperCache::TraceJSObjectFromBits(JSTracer *aTrc, const char *aName)
>+{
>+  JS_CallMaskedObjectTracer(aTrc, &mWrapperPtrBits, kWrapperBitMask, aName);
>+}
nsWrapperCache::TraceJSObjectFromBits(JSTracer *aTrc, const char *aName)
* goes with the type
>     for (const ListenerData* listenerElem = collection->mListeners.getFirst();
>          listenerElem;
>          listenerElem = listenerElem->getNext()) {
>       JS_CallObjectTracer(aTrc,
>-                          listenerElem->mListener,
>+                          &const_cast<ListenerData *>(listenerElem)->mListener,
<ListenerData*>


>   jsid id = NPIdentifierToJSId(memberPrivate->methodName);
>-  JS_CallIdTracer(trc, id, "NPObjectMemberPrivate.methodName");
>+  JS_CallIdTracer(trc, &id, "NPObjectMemberPrivate.methodName");
>+  memberPrivate->methodName = JSIdToNPIdentifier(id);

I don't actually know about this stuff. Someone more familiar with plugins should review. jst or bsmedberg perhaps-
Attachment #731376 - Flags: review?(bugs) → review+
Comment on attachment 731376 [details] [diff] [review]
part 2 of 2: v1 - Rewritten from scratch.

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

::: dom/workers/EventListenerManager.cpp
@@ +184,5 @@
>      for (const ListenerData* listenerElem = collection->mListeners.getFirst();
>           listenerElem;
>           listenerElem = listenerElem->getNext()) {
>        JS_CallObjectTracer(aTrc,
> +                          &const_cast<ListenerData *>(listenerElem)->mListener,

Can you get rid of the cast by dropping some 'const's above?
Comment on attachment 731370 [details] [diff] [review]
part 1 of 2: v0 JS API changes for indirect tracing.

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

::: js/public/GCAPI.h
@@ +213,5 @@
>          value = obj;
>          return *this;
>      }
>  
> +    JSObject **address() { return &value; }

Instead of this, could you add a trace method that gets passed a JSTracer* and a description? It would just call JS_CallObjectTracer.

::: js/src/jsapi.h
@@ +2550,5 @@
>  # define JS_SET_TRACING_NAME(trc, name)                                       \
>      JS_SET_TRACING_DETAILS(trc, NULL, name, (size_t)-1)
>  
> +/*
> + * The JS_Call*Tracer family of functions traces the given gc thing reference.

GC should be capitalized.

@@ +2555,5 @@
> + * This performs the tracing action configured on the given JSTracer:
> + * typically calling the JSTracer::callback or marking the thing as live.
> + *
> + * The argument to JS_Call*Tracer is an in-out param: when the function
> + * returns, the garbage collector might have moved the gc thing. In this case,

same here

@@ +2556,5 @@
> + * typically calling the JSTracer::callback or marking the thing as live.
> + *
> + * The argument to JS_Call*Tracer is an in-out param: when the function
> + * returns, the garbage collector might have moved the gc thing. In this case,
> + * the reference passed to JS_IsAboutToBeFinalized will be updated to the

Replace JS_IsAboutToBeFinalized with JS_Call*Tracer.

@@ +2581,5 @@
>  extern JS_PUBLIC_API(void)
>  JS_CallGenericTracer(JSTracer *trc, void *gcthing, const char *name);
>  
>  /*
> + * The JS_CallMaskedObjectTracer variant traces a JSObject* which is stored

Replace which with that.
Attachment #731370 - Flags: review?(wmccloskey) → review+
Comment on attachment 731989 [details] [diff] [review]
Move HashSet marker internal.

This seems fine. You should probably replace extern JS_PUBLIC_API(void) with inline void. I'm guessing that would fix the compile error.

Also, it would be nice to assert that the key is in the table. Alternately, perhaps you could pass in the Enum rather than the key.
Attachment #731989 - Flags: feedback?(wmccloskey) → feedback+
Olli asked me to split out the tracing code in dom/plugin/ to a different patch so that someone more familiar with this code can review.

Summary: We need JS tracing to be able to move objects to allow us to support more sophisticated GC algorithms. Thus, we are making the JS tracing interfaces indirect: the traced pointer is now an in/out parameter. Therefore, when the GC thing is stored in a non-standard encoding, as the jsid in this patch seems to be, we need to perform awkward, manual re-encoding and write-back.
Attachment #738164 - Flags: review?(jst)
Comment on attachment 738164 [details] [diff] [review]
part 3 of 2: split out plugin bits per Olli's request.

r=jst
Attachment #738164 - Flags: review?(jst) → review+
https://hg.mozilla.org/mozilla-central/rev/fae8d9338eb5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.