Last Comment Bug 744579 - GC: Make Relocatable HeapPtr for non-post-barriered heap guards
: GC: Make Relocatable HeapPtr for non-post-barriered heap guards
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on:
Blocks: 744270 744612 744615 745325
  Show dependency treegraph
 
Reported: 2012-04-11 14:16 PDT by Terrence Cole [:terrence]
Modified: 2012-05-19 06:41 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (21.94 KB, patch)
2012-04-11 14:40 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v1: ported 8 days into the future. (20.82 KB, patch)
2012-04-19 14:17 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v2: A complete rewrite. (10.02 KB, patch)
2012-05-15 17:23 PDT, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-04-11 14:16:24 PDT
This is needed for storage of HeapPtr (like things) in places where the memory may be moved outside the control of the GC.  This also converts weakmaps to ensure that the abstraction works correctly, although there are more users that need this treatment as well.
Comment 1 Terrence Cole [:terrence] 2012-04-11 14:40:00 PDT
Created attachment 614177 [details] [diff] [review]
v0

This turned out much cleaner than I expected.  I was able to convert DefaultHasher fully to a RelocatablePtr, so I think this might actually be everything except for sharpObjectMap.
Comment 2 Terrence Cole [:terrence] 2012-04-19 14:17:33 PDT
Created attachment 616744 [details] [diff] [review]
v1: ported 8 days into the future.

Rebased and fixed up the MOZ_DELETE and default override weirdness.
Comment 3 Terrence Cole [:terrence] 2012-05-15 17:23:34 PDT
Created attachment 624251 [details] [diff] [review]
v2: A complete rewrite.

This one has actually been verified to work.
Comment 4 Bill McCloskey (:billm) 2012-05-16 16:51:06 PDT
Comment on attachment 624251 [details] [diff] [review]
v2: A complete rewrite.

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

::: js/src/gc/Barrier.h
@@ +235,5 @@
>                       HeapPtr<T2> &v2, T2 *val2);
>  };
>  
> +template <class T>
> +class RelocatablePtr : public EncapsulatedPtr<T>

Would it be possible for RelocatablePtr to derive from HeapPtr? It seems like the only difference is in the destructor. Then you could take out the operator= implementations.

@@ +242,5 @@
> +    RelocatablePtr() : EncapsulatedPtr<T>(NULL) {}
> +    explicit RelocatablePtr(T *v) : EncapsulatedPtr<T>(v) { post(); }
> +    explicit RelocatablePtr(const RelocatablePtr<T> &v)
> +      : EncapsulatedPtr<T>(v) { post(); }
> +    ~RelocatablePtr() {

Could you put an empty line before ~RelocatablePtr?

@@ +263,5 @@
> +        post();
> +        return *this;
> +    }
> +
> +    operator T*() const { return this->value; }

Why is this coercion operator needed? Isn't it inherited from EncapsulatedValue?
Comment 5 Terrence Cole [:terrence] 2012-05-17 11:50:47 PDT
(In reply to Bill McCloskey (:billm) from comment #4)
> Would it be possible for RelocatablePtr to derive from HeapPtr? It seems
> like the only difference is in the destructor. Then you could take out the
> operator= implementations.

After giving it more thought, I don't think so.  I don't think it is safe to allow implicit conversions between HeapPtr* and RelocatatablePtr*, since we need different post barriers.  Also, because of the different post barrier call, I don't think it's okay to share the operator= in any case.

In holly, I've made the RelocatablePtr::post() call T::writeBarrierRelocPost and added such a call to ObjectImpl next to JSObject::writeBarrierPost, which appears to be working.
Comment 6 Terrence Cole [:terrence] 2012-05-18 17:42:48 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/21c6e51e2cb7
Comment 7 Matt Brubeck (:mbrubeck) 2012-05-19 06:41:25 PDT
https://hg.mozilla.org/mozilla-central/rev/21c6e51e2cb7

Note You need to log in before you can comment on or make changes to this bug.