875872 - Add public Heap<T> class for implementing post-barriers in the browser

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: Patch v0

Here's a patch to add template class Heap<T>, which performs post-barriering and relocations, for use in browser code.

Tested by converting the indirect tracer for JSScript* to take a Heap<JSScript*>* and fixing the one location that it found, nsXULPrototypeScript::mScriptObject.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 753895 [details] [diff] [review]
Patch v0

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

I'm not sure if you meant to not set the review flag on this, however what you have here is truly excellent so I went ahead and reviewed it anyway. Some comments are inline, but r=me with or without the requested code motion.

::: js/public/RootingAPI.h
@@ +174,5 @@
> + *
> + * Implements post barriers for heap-based GC thing pointers outside the engine.
> + */
> +template <typename T>
> +class Heap : public js::HeapBase<T>

I guess this logically belongs more in js/public/HeapAPI.h than here, but I'm not really sure how we could accomplish that without making everything more painful. Re-using RootMethods certainly makes sense and we absolutely need the implicit Handle constructor.

@@ +177,5 @@
> +template <typename T>
> +class Heap : public js::HeapBase<T>
> +{
> +  public:
> +    Heap() { set(js::RootMethods<T>::initial()); }

I guess conceptually RootMethods is really more of a GCMethods now. At some point (e.g. not right now) we should probably move RootMethods into GCAPI.h and give HeapPtr the same nice uniformity.

@@ +271,5 @@
>      Handle(MutableHandle<T> handle) {
>          ptr = handle.address();
>      }
>  
> +    Handle(const Heap<T> &heapPtr) {

Handle is probably going to want an operator= as well. Thankfully, we'll probably be able to get away without needing SFINAE, since we don't expose any internal types to the browser.

::: js/public/Value.h
@@ +1499,5 @@
> + * Augment the generic Heap<T> interface when T = Value with type-querying
> + * and value-extracting operations.
> + */
> +template <>
> +class HeapBase<JS::Value> : public ValueOperations<JS::Heap<JS::Value> >

\o/ Brilliant composition!

::: js/src/jsapi.cpp
@@ +2550,5 @@
> +{
> +    MarkScriptUnbarriered(trc, scriptp->unsafeGet(), name);
> +}
> +
> +#ifdef JSGC_GENERATIONAL

I think we should keep the declarations for JS::HeapFoo{PostBarrier|Relocate} in gc/StoreBuffer.cpp, if possible.

::: js/src/jsapi.h
@@ +2535,5 @@
>  extern JS_PUBLIC_API(void)
>  JS_CallScriptTracer(JSTracer *trc, JSScript **scriptp, const char *name);
>  
>  extern JS_PUBLIC_API(void)
> +JS_CallHeapValueTracer(JSTracer *trc, JS::Heap<JS::Value> *valuep, const char *name);

Eventually I'd like to have the non-barriered variants be the special one (e.g. JS_CallValueTracerUnbarriered(JSTracer*, Value*, char*)). We can change the names to that form as we convert each type: adding "Heap" is fine in the meantime to keep them distinct.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #1)

Great, thanks for the comments!   Yes, I did mean to r? you on it.

Comment 3

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/909d9dd8ff2f

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/909d9dd8ff2f