GC: Make Relocatable HeapPtr for non-post-barriered heap guards

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #614177 - Flags: review?(wmccloskey)
(Assignee)

Updated

5 years ago
Blocks: 673454
(Assignee)

Updated

5 years ago
Blocks: 744270
No longer blocks: 673454
(Assignee)

Updated

5 years ago
Blocks: 744612
(Assignee)

Updated

5 years ago
Blocks: 744615
(Assignee)

Updated

5 years ago
Blocks: 745325
(Assignee)

Comment 2

5 years ago
Created attachment 616744 [details] [diff] [review]
v1: ported 8 days into the future.

Rebased and fixed up the MOZ_DELETE and default override weirdness.
Attachment #614177 - Attachment is obsolete: true
Attachment #614177 - Flags: review?(wmccloskey)
Attachment #616744 - Flags: review?(wmccloskey)
(Assignee)

Updated

5 years ago
Attachment #616744 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

5 years ago
Created attachment 624251 [details] [diff] [review]
v2: A complete rewrite.

This one has actually been verified to work.
Attachment #616744 - Attachment is obsolete: true
Attachment #624251 - Flags: review?(wmccloskey)
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?
Attachment #624251 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/21c6e51e2cb7
https://hg.mozilla.org/mozilla-central/rev/21c6e51e2cb7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.