Bug 236894 – Explain visit children and opaque roots in Introduction.md

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Add descriptions for these.

[Ryosuke Niwa]

Created attachment 452655 [details]
Patch

[Alexey Shvayka]

Comment on attachment 452655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452655&action=review

Wow, this is so useful, thank you for the write-up!

> Introduction.md:731
> +[WebCore](https://github.com/WebKit/WebKit/tree/main/Source/WebCore) contains hundreds of [Web IDL](https://heycam.github.io/webidl/) (.idl) files.

nit: Web IDL spec was moved to more canonical URL: https://webidl.spec.whatwg.org

[Ryosuke Niwa]

Comment on attachment 452655 [details]
Patch

There is a zero chance this patch would have caused the reported test failures.

[Saam Barati]

Comment on attachment 452655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452655&action=review

> Introduction.md:807
> +*Visit Children* is useful when a JS wrapper needs to keep another JS wrapper or JS object alive.

I'd say something like:
Visit Children is the mechanism we use when a JS Wrapper needs to keep another JS wrapper or JS cell alive.

Particularly, using "useful" doesn't quite capture the necessity.

> Introduction.md:846
> +Note that [JSValueInWrappedObject](https://github.com/WebKit/WebKit/blob/main/Source/WebCore/bindings/js/JSValueInWrappedObject.h)
> +is a weak reference to an JS object.
> +We can't use a strong reference here since the stored JS object may also have a reference back to this object,
> +and such a back reference would also strongly keep this `ErrorEvent` object alive.
> +Because the garbage collector has no way of knowing or clearing such a strong reference in this hypothetical version of `ErrorEvent`,
> +it would never be able to collect either object, resulting in memory leak.

I think we need to come up with different terminology when speaking of GC references. Notably, really aren't using a weak reference here, since we're visiting it (and presumably we're also emitting a write barrier when storing to it).

So, saying this is Weak is wrong. I'm not sure what the exact terminology we should use is, but I don't think the language here is right. Maybe we can do something like:
- "Weak reference". Actual weak reference. We make no attempts to keep this reference alive. (Notably, not the "m_error" field above, since we visit it.)
- "GC reference". We keep the thing pointed to alive as long as the thing pointing to it is alive. If X points to Y, and X is alive, then Y must also be alive. (I think a lot of literature might actually use the terminology "Strong" to refer to this kind of reference.)
- "Root reference" or "Strong" reference. This means that we keep the thing being pointed to alive as long as the C++ reference is alive. JSC implements this via the "Strong<T>" class. This is a "Root" in the classical GC sense.

We should probably define what we mean in this document before using the terminology.

> Introduction.md:859
> +*Reachable from Opaque Roots* is useful when we have an underlying C++ object and want to keep JS wrappers of other C++ objects alive.

nit: again, I'd find a different way of saying this than "useful". Maybe just "... is used when we have..."

> Introduction.md:874
> +*Opaque roots* avoid these problems by letting the garbage collector know that a particular JavaScript wrapper needs to be kept alive

nit: "avoid these problems" => "allows us to solve these problems"?

> Introduction.md:929
> + 1. Add opque roots in `visitAdditionalChildren`.

opque => opaque

> Introduction.md:996
> +Similar to visiting children or adding opaque roots,

nit: this sentence is kinda confusing with starting with how they're similar and ending the sentence with how they're different. I think I'd just call out the differences. Or perhaps break it up into two sentences?

> Introduction.md:997
> +**an opaque root is reachable or not is checked in parallel** but **while the main is paused**

an => whether an
main is => main thread is

[Ryosuke Niwa]

(In reply to Saam Barati from comment #4)
> Comment on attachment 452655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452655&action=review
> 
> > Introduction.md:807
> > +*Visit Children* is useful when a JS wrapper needs to keep another JS wrapper or JS object alive.
> 
> I'd say something like:
> Visit Children is the mechanism we use when a JS Wrapper needs to keep
> another JS wrapper or JS cell alive.
> 
> Particularly, using "useful" doesn't quite capture the necessity.

Sure, I'm gonna stick to "JS object" here though since most people working on WebCore aren't (and they don't need to be) familiar with the notion of a cell.

> > Introduction.md:846
> > +Note that [JSValueInWrappedObject](https://github.com/WebKit/WebKit/blob/main/Source/WebCore/bindings/js/JSValueInWrappedObject.h)
> > +is a weak reference to an JS object.
> > +We can't use a strong reference here since the stored JS object may also have a reference back to this object,
> > +and such a back reference would also strongly keep this `ErrorEvent` object alive.
> > +Because the garbage collector has no way of knowing or clearing such a strong reference in this hypothetical version of `ErrorEvent`,
> > +it would never be able to collect either object, resulting in memory leak.
> 
> I think we need to come up with different terminology when speaking of GC
> references. Notably, really aren't using a weak reference here, since we're
> visiting it (and presumably we're also emitting a write barrier when storing
> to it).
> 
> So, saying this is Weak is wrong. I'm not sure what the exact terminology we
> should use is, but I don't think the language here is right. Maybe we can do
> something like:
> - "Weak reference". Actual weak reference. We make no attempts to keep this
> reference alive. (Notably, not the "m_error" field above, since we visit it.)
> - "GC reference". We keep the thing pointed to alive as long as the thing
> pointing to it is alive. If X points to Y, and X is alive, then Y must also
> be alive. (I think a lot of literature might actually use the terminology
> "Strong" to refer to this kind of reference.)
> - "Root reference" or "Strong" reference. This means that we keep the thing
> being pointed to alive as long as the C++ reference is alive. JSC implements
> this via the "Strong<T>" class. This is a "Root" in the classical GC sense.

That's an interesting point. I guess there is a reference that's weak in its conceptual level vs. what's mechanically Weak vs. Strong.

I've rephrased as follows:
Note that [`JSValueInWrappedObject`](https://github.com/WebKit/WebKit/blob/main/Source/WebCore/bindings/js/JSValueInWrappedObject.h)
uses [`Weak`](https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/heap/Weak.h),
which does not keep the referenced object alive on its own.
We can't use a reference type such as [`Strong`](https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/heap/Strong.h)
which keeps the referenced object alive on its own since the stored JS object may also have this `ErrorEvent` object stored as its property.
Because the garbage collector has no way of knowing or clearing the `Strong` reference
or the property to `ErrorEvent` in this hypothetical version of `ErrorEvent`,
it would never be able to collect either object, resulting in a memory leak.

> > Introduction.md:859
> > +*Reachable from Opaque Roots* is useful when we have an underlying C++ object and want to keep JS wrappers of other C++ objects alive.
> 
> nit: again, I'd find a different way of saying this than "useful". Maybe
> just "... is used when we have..."

Sure, fixed.

> > Introduction.md:874
> > +*Opaque roots* avoid these problems by letting the garbage collector know that a particular JavaScript wrapper needs to be kept alive
> 
> nit: "avoid these problems" => "allows us to solve these problems"?

I'd just say "*Opaque roots* solves these problems by"

> > Introduction.md:929
> > + 1. Add opque roots in `visitAdditionalChildren`.
> 
> opque => opaque

Fixed.

> > Introduction.md:996
> > +Similar to visiting children or adding opaque roots,
> 
> nit: this sentence is kinda confusing with starting with how they're similar
> and ending the sentence with how they're different. I think I'd just call
> out the differences. Or perhaps break it up into two sentences?

Okay, split into two sentences:

Similar to visiting children or adding opaque roots, **an opaque root is reachable or not is checked in parallel**.
However, it happens **while the main is paused** unlike visiting children or adding opaque roots,
which happen concurrently while the main thread is running.

> > Introduction.md:997
> > +**an opaque root is reachable or not is checked in parallel** but **while the main is paused**
> 
> an => whether an
> main is => main thread is

Fixed.

[Ryosuke Niwa]

Created attachment 453126 [details]
Addresed review comments

[Saam Barati]

Comment on attachment 453126 [details]
Addresed review comments

r=me

[EWS]

Committed r290463 (247762@main): <https://commits.webkit.org/247762@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453126 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89439787>