Closed Bug 877762 Opened 11 years ago Closed 11 years ago

GC: Post-barrier cycle collector participants

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(9 files, 6 obsolete files)

11.36 KB, patch
terrence
: review+
Details | Diff | Splinter Review
25.20 KB, patch
justin.lebar+bug
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
5.81 KB, patch
smaug
: review+
Details | Diff | Splinter Review
29.38 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
27.63 KB, patch
bzbarsky
: review+
sfink
: review+
Details | Diff | Splinter Review
2.32 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
26.22 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.72 KB, patch
Details | Diff | Splinter Review
      No description provided.
Attached patch 1 - Updates to JS::Heap<T> (obsolete) — Splinter Review
I've uploaded my WIP patches.  There are a few outstanding issues:

 - nsTArray<JS::Value> use is bodged at the moment.  It compiles but is broken because nsTArray uses memcpy/memmove internally.  I'm trying to work out what's the best of the several reasonably unpleasant ways to handle this.
 - nsWrapperCache needs us to correctly relocate masked pointers that store flags in their unused bits.  I put in hooks to call into the store buffer code for these, but have not implmemented them
 - JSObject conversion not done yet.
What is the high-level purpose of these patches?  What is the barrier doing?
(In reply to Andrew McCreight [:mccr8] from comment #6)
> What is the high-level purpose of these patches?  What is the barrier doing?

These tell us where pointers in non-nursery heaps point to things in the nursery. This allows us to find them and update then when we do a minor collection. The alternative is to trace the browser heap for minor collections. Since this takes an order of magnitude longer than the rest of the minor GC, barriers are our only real option.
Okay.  And this is being accomplished by changing raw pointers to JS Foo to  JS::Heap<Foo> which adds the barrier stuff when you set it?
Comment on attachment 756632 [details] [diff] [review]
1 - Updates to JS::Heap<T>

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

This looks fine, but I'd like to get Waldo's thoughts on the Value.h changes too.

::: js/public/RootingAPI.h
@@ +215,5 @@
>          }
>      }
>  
>    private:
> +    void init(T newPtr) {

I guess we just never ran into a case in spidermonkey where we initialize a Relocatable after the constructor.
(In reply to Andrew McCreight [:mccr8] from comment #8)
> Okay.  And this is being accomplished by changing raw pointers to JS Foo to 
> JS::Heap<Foo> which adds the barrier stuff when you set it?

Exactly!  And we catch all the JSFoo* that need this by changing the signature of the trace functions. Also, that reminds me that we are going to need to give the same type change to JS_IsAboutToBeFinalized. I'll go file it.
Comment on attachment 756632 [details] [diff] [review]
1 - Updates to JS::Heap<T>

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

Actually, if we look at the bigger picture, it's pretty clear that we are off in the woods with this.

r=me with the change below.

::: js/public/RootingAPI.h
@@ +215,5 @@
>          }
>      }
>  
>    private:
> +    void init(T newPtr) {

I guess we just never ran into a case in spidermonkey where we initialize a Relocatable after the constructor.

::: js/public/Value.h
@@ +1548,5 @@
> +        if (arg)
> +            setObject(*arg);
> +        else
> +            setNull();
> +    }

Actually, I think we can do without most of this. In the case where the Value is going from a non-object to a non-object, no barrier is needed. If the Value is going from an object to a non-object, then we will still call MarkValue on it, but MarkValue will do nothing: this is fine as the object hasn't moved. If the object did move, the destructor would still be called on it. If we move from an non-object to an object, then we must call the post barrier. Thus, I think the only methods we really need here are setObject, setObjectOrNull, and setString as well, so we are future-proof.
Attachment #756632 - Flags: review+
Depends on: 860573
Comment on attachment 756632 [details] [diff] [review]
1 - Updates to JS::Heap<T>

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

Just saw the comment -- if you want my attention, needinfo or review or feedback is better than just mentioning me.  :-)

::: js/public/Value.h
@@ +1512,5 @@
> +        static_cast<JS::Heap<JS::Value> *>(this)->set(v);
> +    }
> +
> +  public:
> +    void setNull() { setImpl(JS::NullValue()); }

Hmm.  We have ValueOperations precisely so we *don't* have to repeat this laundry list of methods multiple places.  There's surely a better way to do this.  Something like changing this in MutableValueOperations

    JS::Value * value() { return static_cast<Outer*>(this)->extractMutable(); }

to this

    void setValueImpl(JS::Value v) {
        static_cast<Outer*>(this)->setValueImpl(v);
    }
    bool setNumberImpl(uint32_t ui) {
        return static_cast<Outer*>(this)->setNumberImpl(ui);
    }
    bool setNumberImpl(double d) {
        return static_cast<Outer*>(this)->setNumberImpl(d);
    }

and then change all classes inheriting from MVO to have private setValueImpl/setNumberImpl methods (which MVO can access, because classes inheriting from MVO have to friend MVO) that do the right thing?  Definitely we don't want to repeat this list of methods, tho.
Attachment #756632 - Attachment is obsolete: true
Attachment #756633 - Attachment is obsolete: true
Attachment #756635 - Attachment is obsolete: true
Attachment #756636 - Attachment is obsolete: true
Attachment #760999 - Attachment description: Convert JS::Object to use Heap<T> (XBL) → 7 - Convert JS::Object to use Heap<T> (XBL)
Attachment #760985 - Flags: review?(terrence)
Try build here: https://tbpl.mozilla.org/?tree=Try&rev=5114c659929b

I'll request review for the other patches if this turns out ok.
Comment on attachment 760985 [details] [diff] [review]
1 - Updates to JS::Heap<T>

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

Looks good!

r=me

::: js/public/Value.h
@@ +1530,5 @@
> +     * Setters that potentially change the value to a GC thing from a non-GC
> +     * thing must call JS::Heap::set() to trigger the post barrier.
> +     *
> +     * Changing from a GC thing to a non-GC thing value will leave the heap
> +     * value in the store buffer, but it will be ingored so this is not a

Missing n in 'igored'.
Attachment #760985 - Flags: review?(terrence) → review+
The purpose of this bug is to add post-barriers for JC GC thing pointers stored on the browser heap.  Post barriers are a way of informing the GC whenever a GC thing pointer is updated, and are necessary for generational GC to function.

We do this by replacing pointers with the JS::Heap<T> wrapper class.  This uses C++ operator overloading to catch assignments and updates, so for the most part all that needs to be done is update the data member definitions.

We update the TraceCallbacks methods to take pointers to JS::Heap<T>s so that the type system ensures we have done this everywhere.
Comment on attachment 760986 [details] [diff] [review]
2 - Make nsTArray move Heap<T> elements safely

This patch allows JS::Heap<T>s to be stored in nsTArray type arrays.  This is necessary because nsTArray references are returned from some APIs, and changing these would be problematic.

The issue with nsTArray as it stands is that it uses memcpy and memmove to move its contents when resizing.  Heap<T>s however register their address with the GC and need to be destructed/constructed when moved to a new location.

The changes here abstract the copying behaviour into a separate class, which uses template specialization to do the right thing when the element type is a Heap<T>.  

There are also changes to add conversions from nsTArray<Heap<T>> to const nsTArray<T>&s, which is safe as Heap<T> and T share the same representation.
Attachment #760986 - Flags: review?(justin.lebar+bug)
Comment on attachment 760988 [details] [diff] [review]
3 - Add hashtable type that stores Heap<T> values

This patch adds a hashtable type where the values are JS GC thing pointers.  These are stored as JS::Heap<T>s, while the callers just see the raw T.
Attachment #760988 - Flags: review?(justin.lebar+bug)
Comment on attachment 760991 [details] [diff] [review]
3 - Convert JSScript to use Heap<T>

Replace use of JSScript* cycle collector participants with JS::Heap<JSScript*>.
Attachment #760991 - Flags: review?(bugs)
Comment on attachment 760993 [details] [diff] [review]
4 - Convert JS::Value to use Heap<T>

Replace use of JS::Value cycle collector participants with JS::Heap<JS::Value>.
Attachment #760993 - Flags: review?(bugs)
Comment on attachment 760994 [details] [diff] [review]
5 - Convert jsid and JSString to use Heap<T>

Replace use of JSString* and jsid cycle collector participants with JS::Heap<jsid>.
Attachment #760994 - Flags: review?(bugs)
Comment on attachment 760998 [details] [diff] [review]
6 - Convert JS::Obect to use Heap<T> (ex. XBL)

Replace use of JSObject* cycle collector participants with JS::Heap<JSObject*>, excluding changes in content/xbl.
Attachment #760998 - Flags: review?(bzbarsky)
Comment on attachment 760999 [details] [diff] [review]
7 - Convert JSObject to use Heap<T> (XBL)

Replace use of JSObject* cycle collector participants with JS::Heap<JSObject*> in content/xbl.

This is slightly complicate as it preserves the current union setup where compiled functions and their uncompiled source share the same storage.  Another simpler possibility would be to separate these members.  I don't know whether that would be a significant memory increase or not.
Attachment #760999 - Flags: review?(bzbarsky)
I don't follow part 3.  That allows memmoving the hashtable entries, no?
Comment on attachment 760998 [details] [diff] [review]
6 - Convert JS::Obect to use Heap<T> (ex. XBL)

OK, so assuming the jsthing hashtable ends up working as it should...

>+++ b/content/base/src/nsDocument.cpp

What about mExpandoAndGeneration.expando ?

>+++ b/dom/base/nsWrapperCache.cpp

Why did TraceWrapper have to get uninlined?

>+++ b/dom/indexedDB/IDBCursor.h

What about mCachedKey, mCachedPrimaryKey, mCachedValue?

What about IDBKeyRange::mCachedLowerVal, IDBKeyRange::mCachedUpperVal, IDBIndex::mCachedKeyPath, IDBObjectStore::mCachedKeyPath, IDBRequest::mResultVal?

Other things that seem to be missing here:

nsXMLHttpRequest::mResultJSON, nsDOMMessageEvent::mData, nsHTMLDocument::mAll, nsXULPrototypeScript::mScriptObject, DOMRequest::mResult, nsJSARGArray::mArgv[n], nsJSScriptTimeoutHandler::mExpr, nsJSScriptTimeoutHandler::mArgs[n], BluetoothAdapter::mJSUuids, BluetoothAdapter::mJsDeviceAddresses, BlueToothDevice::mJSUuids, BlueToothDevice::mJSServices, Future::mResult, Telephony::mCallsArray.
Attachment #760998 - Flags: review?(bzbarsky) → review-
Comment on attachment 760999 [details] [diff] [review]
7 - Convert JSObject to use Heap<T> (XBL)

Some general comments:  The Compiled() thing looks too much like a boolean getter.  How about calling it GetJSFunction() instead?  Also Uncompiled() should be GetUncompiled(): lack of a "Get" prefix means return value is never null in Gecko.

>+ * The purpose of abstracting this as a sparate class is to allow it to be

"separate"

>+private:
>+  JSObject*& UnsafeCompiled()

You need some friend declarations or something to make this compile when defined(JSGC_GENERATIONAL)....

>+    Base::postBarrier(functionp->UnsafeCompiled());

Shouldn't that be Base::PostBarrier(&functionp->UnsafeCompiled()); or something?  At which point, why not rename UnsafeCompiled to CompiledSlot and just have it return a JSObject**?

>+  JSObject* jsMethodObject = GetCompiledMethod();

This needs to be a Rooted, because JSAutoCompartment can GC and move stuff.  Alternately, don't use a temporary at all here and just reget from our traced slot like you do elsewhere.

>+++ b/content/xbl/src/nsXBLProtoImplMethod.h
>+ protected:

Outdent by one space, please.

> protected:

And remove this bit?

>+++ b/content/xbl/src/nsXBLProtoImplProperty.cpp
>+    if (!text)
>+      return;

Can't happen, which is good because just returning from an Ensure* method is bogus.  Please just remove those two lines.

>+++ b/content/xbl/src/nsXBLProtoImplProperty.h

>+  typedef JS::Heap<nsXBLMaybeCompiled<nsXBLTextWithLineNumber> > GetterSetter;

How about calling that PropertyOp?

Someone who actually knows the JS template stuff should review the namespace js bits of nsXBLMaybeCompiled.h.

r=me with the above issues fixed.
Attachment #760999 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #31)
> I don't follow part 3.  That allows memmoving the hashtable entries, no?

You're right, I missed that.
Attachment #760988 - Flags: review?(justin.lebar+bug)
Attachment #760991 - Flags: review?(bugs) → review+
Comment on attachment 760991 [details] [diff] [review]
3 - Convert JSScript to use Heap<T>

>-    virtual void Trace(JSScript **p, const char *name, void *closure) const MOZ_OVERRIDE {
>-        JS_CallScriptTracer(static_cast<JSTracer*>(closure), p, name);
>+    virtual void Trace(JS::Heap<JSScript *> *p, const char *name, void *closure) const MOZ_OVERRIDE {
Extra space after JSScript
Comment on attachment 760993 [details] [diff] [review]
4 - Convert JS::Value to use Heap<T>

>   // The Unicode converter has already zapped the BOM if there was one
>+  JS::Rooted<JS::Value> value(aCx);
>   if (!JS_ParseJSON(aCx,
>                     static_cast<const jschar*>(mResponseText.get()), mResponseText.Length(),
>-                    JS::MutableHandle<JS::Value>::fromMarkedLocation(&mResultJSON))) {
>+                    &value)) {

So, as I mentioned on IRC, requirement to create temporary Rooted for this kind of cases is rather ugly.
Hopefully we can figure out some better solution.

>-  nsTArray<JS::Value> mArgs;
>+  nsTArray<JS::Heap<JS::Value>> mArgs;
space between > and >

>+    FallibleTArray<JS::Heap<JS::Value>> args;
ditto

> GetKeyHelper::GetSuccessResult(JSContext* aCx,
>                                jsval* aVal)
> {
>-  return mKey.ToJSVal(aCx, aVal);
>+  // todo?
What is this todo about?

>+  JS::Rooted<JS::Value> value(aCx);
>+  nsresult rv = mKey.ToJSVal(aCx, &value);
>+  if (NS_SUCCEEDED(rv))
>+    *aVal = value;
{}


>@@ -3128,17 +3128,22 @@ AddHelper::DoDatabaseWork(mozIStorageCon
> nsresult
> AddHelper::GetSuccessResult(JSContext* aCx,
>                             jsval* aVal)
> {
>   NS_ASSERTION(!mKey.IsUnset(), "Badness!");
> 
>   mCloneWriteInfo.mCloneBuffer.clear();
> 
>-  return mKey.ToJSVal(aCx, aVal);
>+  // todo?
?

>+  JS::Rooted<JS::Value> value(aCx);
>+  nsresult rv = mKey.ToJSVal(aCx, &value);
>+  if (NS_SUCCEEDED(rv))
>+    *aVal = value;
{}

...so, per IRC those todos are about MHV. Could be a followup bug or patch to make this
stuff to use MHV

>+  nsresult ToJSVal(JSContext* aCx,
>+                   JS::Heap<JS::Value>& aVal) const
>+  {
>+    JS::Rooted<JS::Value> value(aCx);
>+    nsresult rv = ToJSVal(aCx, &value);
>+    if (NS_SUCCEEDED(rv))
>+      aVal = value;
{}

>+KeyPath::ToJSVal(JSContext* aCx, JS::Heap<JS::Value>& aValue) const
>+{
>+  JS::Rooted<JS::Value> value(aCx);
>+  nsresult rv = ToJSVal(aCx, &value);
>+  if (NS_SUCCEEDED(rv))
>+    aValue = value;
{}
Attachment #760993 - Flags: review?(bugs) → review+
Attachment #760994 - Flags: review?(bugs) → review+
(In reply to Boris Zbarsky (:bz) from comment #33)

Thanks for all the comments.

I should have explained this, but I split the changes up by type, so the JSObject* changes are in one patch, the JS::Value ones in another etc.  So, most of the JS members you mention in comment 32 are handled by the other patches.  In hindsight it would have been better to rearrange these by component for review.

I did miss the telephony and bluetooth ones, and nsHTMLDocument::mAll and the Future::mResult are new since I made these patches.  I'll fix these and update the bug.

TraceWrapper - for some reason I was getting build errors on MacOS with this inline. I don't remember the reason.  I'll try and put it back the way it was. 

I've renamed the methods in content/xbl as suggested.
Comment on attachment 760999 [details] [diff] [review]
7 - Convert JSObject to use Heap<T> (XBL)

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

Looks ok to me, though it makes me wonder: is space such a big concern? Wouldn't this be simpler if you got rid of the union and just kept both fields? Then tests like

  if (IsCompiled() && GetCompiledMethod())

could just be

  if (mJSMethodObject)

and you could use a plain HeapPtr<JSObject*> and not need the nsXBLMaybeCompiled at all. Or is there some other reason for the additional abstraction?

::: content/xbl/src/nsXBLMaybeCompiled.h
@@ +77,5 @@
> +  static nsXBLMaybeCompiled<UncompiledT> initial() { return nsXBLMaybeCompiled<UncompiledT>(); }
> +
> +  static bool poisoned(nsXBLMaybeCompiled<UncompiledT> function)
> +  {
> +    return function.IsCompiled() && Base::poisoned(function.Compiled()); 

There's a single trailing space on a number of lines in this patch.
Attachment #760999 - Flags: review+
> is space such a big concern? 

Yes.
Attachment #760988 - Attachment is obsolete: true
Attachment #762616 - Flags: review?(justin.lebar+bug)
Updated patch with comments addressed, as described in comment 37.
Attachment #760998 - Attachment is obsolete: true
Attachment #762617 - Flags: review?(bzbarsky)
Attachment #760994 - Attachment description: 5 - Convert jsid and JS::String to use Heap<T> → 5 - Convert jsid and JSString to use Heap<T>
Attachment #762617 - Attachment description: 6 - Convert JS::Obect to use Heap<T> (ex. XBL) → 6 - Convert JSObect to use Heap<T> (ex. XBL)
Attachment #760999 - Attachment description: 7 - Convert JS::Object to use Heap<T> (XBL) → 7 - Convert JSObject to use Heap<T> (XBL)
Attachment #762617 - Attachment description: 6 - Convert JSObect to use Heap<T> (ex. XBL) → 6 - Convert JSObject to use Heap<T> (ex. XBL)
Attachment #762711 - Flags: review?
Comment on attachment 762617 [details] [diff] [review]
6 - Convert JSObject to use Heap<T> (ex. XBL)

r=me assuming you added the Future::mResult bit to the Value patch
Attachment #762617 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #43)

Yes, I have.  Cheers!
Comment on attachment 760986 [details] [diff] [review]
2 - Make nsTArray move Heap<T> elements safely

Looks great to me with some nits.

There's some trailing whitespace in this patch:

> $ git apply <(pbpaste)
> /dev/fd/63:516: trailing whitespace.
> struct nsTArray_CopyWithConstructors 
> /dev/fd/63:538: trailing whitespace.
>     CopyElements(static_cast<uint8_t*>(dest) + sizeof(nsTArrayHeader), 
> /dev/fd/63:539: trailing whitespace.
>                  static_cast<uint8_t*>(src) + sizeof(nsTArrayHeader), 
> warning: 3 lines add whitespace errors.

I have a tool for git if you use that; git-fix-whitespace in
https://github.com/jlebar/moz-git-tools.

In fact, there's a pre-commit hook in there that won't let you commit trailing
whitespace.  :)

>diff --git a/xpcom/glue/nsTArray-inl.h b/xpcom/glue/nsTArray-inl.h
>--- a/xpcom/glue/nsTArray-inl.h
>+++ b/xpcom/glue/nsTArray-inl.h

>-template<class Alloc>
>+template<class Alloc, class Copy>
> template<class Allocator>
> typename Alloc::ResultTypeProxy
>-nsTArray_base<Alloc>::SwapArrayElements(nsTArray_base<Allocator>& other,
>+nsTArray_base<Alloc, Copy>::SwapArrayElements(nsTArray_base<Allocator, Copy>& other,
>                                         size_type elemSize,
>                                         size_t elemAlign) {

Nit: Fix indentation of params above.

>diff --git a/xpcom/glue/nsTArray.h b/xpcom/glue/nsTArray.h
>--- a/xpcom/glue/nsTArray.h
>+++ b/xpcom/glue/nsTArray.h

> public:
>+  typedef Copy copy_type;

Does this typedef need to be public?  I'm not sure whether nsTArray is the
right person to be telling the public whether or not a type is memmoveable.

Separately, do we need this typedef at all?  It would be nice if we could use
Copy directly, like we use Alloc directly.

>+// A template class that defines how to copy elements calling their constructors
>+// and destructors appropriately.

>+template <class ElemType>
>+struct nsTArray_CopyWithConstructors 
>+{

>+  static void MoveElements(void* dest, void* src, size_t count, size_t elemSize) {
>+    ElemType* destElem = static_cast<ElemType*>(dest);
>+    ElemType* srcElem = static_cast<ElemType*>(src);
>+    ElemType* destElemEnd = destElem + count;
>+    ElemType* srcElemEnd = srcElem + count;
>+    if (srcElemEnd > destElem && srcElemEnd <= destElemEnd) {
>+      while (destElemEnd != destElem) {
>+        --destElemEnd;
>+        --srcElemEnd;
>+        traits::Construct(destElemEnd, *srcElemEnd);
>+        traits::Destruct(srcElem);
>+      }
>+    } else {
>+      CopyElements(dest, src, count, elemSize);
>+    }
>+  }
>+};

This will not work right if dest == src.  It so happens that we don't call
memmove() under that circumstance, but that's still a footgun I'd prefer to
avoid.

Since afaict it's valid to call memmove with two copies of the same pointer, we
should probably replicate those semantics here.

>+//
>+// Base class for nsTArray_Impl that is templated on element type and derived
>+// nsTArray_Impl class, to allow extra convsions to be added for specific types.
>+//
>+template <class E, class Derived>
>+struct nsTArray_TypedBase : public nsTArray_SafeElementAtHelper<E, Derived> {};

convsions

>+//
>+// Specialization of nsTArray_TypedBase for arrays containing JS::Heap<E>
>+// elements.
>+//
>+// These conversions are safe because JS::Heap<E> and E share the same
>+// representation, and since the result of the conversions are const references
>+// we won't miss any barriers.
>+//
>+// The static_cast is necessary to obtain the correct address for the derived
>+// class since we are a base class used in multiple inheritance.
>+//
>+template <class E, class Derived>
>+struct nsTArray_TypedBase<JS::Heap<E>, Derived>
>+ : public nsTArray_SafeElementAtHelper<JS::Heap<E>, Derived>
>+{
>+  operator const nsTArray<E>& () {
>+    Derived* self = static_cast<Derived*>(this);
>+    return *reinterpret_cast<nsTArray<E> *>(self);
>+  }
>+
>+  operator const FallibleTArray<E>& () {
>+    Derived* self = static_cast<Derived*>(this);
>+    return *reinterpret_cast<FallibleTArray<E> *>(self);
>+  }
>+};

This is fine with me, but I have no idea whether it's actually safe.  Can we
get someone (bz?) to sign off on this?

> template<class E, class Alloc>
>-class nsTArray_Impl : public nsTArray_base<Alloc>,
>-                      public nsTArray_SafeElementAtHelper<E, nsTArray_Impl<E, Alloc> >
>+class nsTArray_Impl : public nsTArray_base<Alloc, nsTArray_CopyElements<E> >,
>+                      public nsTArray_TypedBase<E, nsTArray_Impl<E, Alloc> >

For simplicity's sake, I'd prefer if nsTArray_TypedBase didn't inherit from
nsTArray_SafeElementAtHelper, unless there's some reason that it has to be this
way.  That is, I think it'd be simpler if nsTArray had three superclasses (one
of which might be empty) instead of two.
Attachment #760986 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 762616 [details] [diff] [review]
3 - Add hashtable type that stores Heap<T> values

r=me, but could you add a comment on nsHashKeyDisallowMemmove indicating that you don't need to use it directly (nsJSThingHashtable does this for you), and add a short example to nsJSThingHashtable demonstrating how it should be used?  It's not entirely straightforward what's going on here.
Attachment #762616 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 760986 [details] [diff] [review]
2 - Make nsTArray move Heap<T> elements safely

bz, if you're happy with this, can you give your seal of approval to the casting between nsTArray<Heap<T>>& and const nsTArray<T>& as suggested by  jlebar in comment 45?
Attachment #760986 - Flags: review?(bzbarsky)
(In reply to Justin Lebar [:jlebar] from comment #45)

Thanks for the comments.

> I think it'd be simpler if nsTArray had three superclasses (one
> of which might be empty) instead of two.

I tried it this way to start with, but on Windows builds it triggered the
assertion about padding in nsAutoTArray at the end of the file.  It turns out
that there's a longstanding issue with MSVC10 where it doesn't optimise more
than one empty base class, so unfortunately it has to be this way.

I fixed the other issues.
Wow, I'm really glad we have that static assert!
Comment on attachment 760986 [details] [diff] [review]
2 - Make nsTArray move Heap<T> elements safely

Can we add some static asserts in one of these conversion operators about how sizeof(E) == sizeof(JS::Heap<E>) ?

r=me with that.
Attachment #760986 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #50)
> Comment on attachment 760986 [details] [diff] [review]
> 2 - Make nsTArray move Heap<T> elements safely
> 
> Can we add some static asserts in one of these conversion operators about
> how sizeof(E) == sizeof(JS::Heap<E>) ?
> 
> r=me with that.

Will the identical static assert I landed in Heap<T> last week work?
https://mxr.mozilla.org/mozilla-central/source/js/public/RootingAPI.h#182
I would like to have an assert here too in case someone changes the other code and the other assert but forgets to update this code.
This caused about 3% increase in the amount of memory the linker takes when performing PGO builds on Windows, which is *very* bad given the number of problems we've had with the linker running out of virtual address space.  A likely reason could be the amount of templated code this adds.  Is there anything that we can do to reduce the amount of template usage in these patches to make the linker consume less memory?

Thanks!
Uhg, that is pretty bad.

Given that we need to intercept each assignment, we really don't have any choice but to inline these as far as possible. Unfortunately, I think that means that switching everything to SHOUTY_MACROS might not actually win us anything, since the same amount of code will be in play.

Could you expand a bit on the potential impact? I don't really have a great feel for what 3% means in practical terms: what does that equate to in time before we hit the ceiling again and startup performance if we have to cull another 3% out of our currently PGO'd code?
Depends on: 939206
Regressions: CVE-2020-26960
You need to log in before you can comment on or make changes to this bug.