Add a post barrier to ReadBarriered

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8667496 [details] [diff] [review]
add_postbarrier_to_readbarriered-v0.diff

Currently, we do not have any non-tenured weakrefs -- global objects are the only ReadBarriered objects that do not get special treatment because of their use as a key. As we move towards needing less special treatment of hashmaps, we're going to want the post barrier here to catch the removed manual barriers.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c8dd44ec02
(Assignee)

Comment 1

3 years ago
Comment on attachment 8667496 [details] [diff] [review]
add_postbarrier_to_readbarriered-v0.diff

Try run is looking good, let's do this!
Attachment #8667496 - Flags: review?(jcoppeard)
Comment on attachment 8667496 [details] [diff] [review]
add_postbarrier_to_readbarriered-v0.diff

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

Looks good.  

We should add a comment somewhere to the effect that this supports post barriers now, and also that this will only work a as hashtable key when the hash code is stable if the wrapped pointer is moved.

::: js/src/gc/Barrier.h
@@ +526,4 @@
>  
>      T get() const {
>          if (!InternalGCMethods<T>::isMarkable(value))
>              return GCMethods<T>::initial();

Do you remember what this is for?
Attachment #8667496 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 3

3 years ago
(In reply to Jon Coppeard (:jonco) from comment #2)
> Comment on attachment 8667496 [details] [diff] [review]
> add_postbarrier_to_readbarriered-v0.diff
> 
> Review of attachment 8667496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.  
> 
> We should add a comment somewhere to the effect that this supports post
> barriers now, and also that this will only work a as hashtable key when the
> hash code is stable if the wrapped pointer is moved.
> 
> ::: js/src/gc/Barrier.h
> @@ +526,4 @@
> >  
> >      T get() const {
> >          if (!InternalGCMethods<T>::isMarkable(value))
> >              return GCMethods<T>::initial();
> 
> Do you remember what this is for?

Great question! At least, I was asking myself exactly that yesterday. Note that this was initialized with nullptr, so the only workable T are currently Cell*s. Thus, the only place this could be possibly have an effect is for uintptr_t(value) < 32, making it an IsNullTaggedPointer. In this case, isMarkable would do find the IsNullTaggedPointer and replace the result with an untagged nullptr.

It used to be the case that IsNullTaggedPointer was only used for JSObject, but it looks like this got moved to the generic isMarkable, so now also works for Shape*. My guess would be that this is to support our use of Shape* == 1 to indicate cases where there is a virtual shape for TypedObject, UnboxedObject and such. The right solution here would have been to create a TaggedShape class and do what we currently have for TaggedObject for marking. At a guess, this was an expedient "fix" to make some random path work with tagged shapes that othewise wouldn't.

Comment 5

3 years ago
Apparently this caused a small regression on a few tests from Kraken and a bigger regression on Octane-RegExp.
Flags: needinfo?(terrence)
(Assignee)

Comment 6

3 years ago
So it did.
Flags: needinfo?(terrence)
(Assignee)

Comment 8

3 years ago
Oddly, I'm seeing a substantially higher score /with/ the patch applied locally.
(Assignee)

Comment 9

3 years ago
And a small, but statistically relevant, bit slower on 32bit builds locally.
(Assignee)

Comment 10

3 years ago
Created attachment 8668611 [details] [diff] [review]
1_rename_BarrieredBase_to_WriteBarrieredBase-v0.diff

I had really, really hoped to avoid it, but since it seems we need a ReadBarriered *and* a TenuredReadBarriered (at the very least), it appears we would be very well served at this point by doing a rethink of our Barrier hierarchy again in light of the new requirements.

We cannot simply derive ReadBarriered from BarrieredBase, because of all the shared, non-read-barriered getters it provides to our write barriers. One semi-obvious solution would be to add read-barriers to BarrieredBase and use CRTP to give it a blank read barrier as needed for write barriers. Sadly (or happily, depending on how you look at it), this runs head into a second problem: BarrieredBase is the class taken by our Tracer functions and we don't want ReadBarriered to be traceable. Also, making this use CRTP (in addition to the mixins) would be a guaranteed way to make the code less comprehensible.

What I've chosen to do instead is to lower BarrieredBase and shim in WriteBarrieredBase and ReadBarrieredBase below our write and read barriers. This allows us to pass WriteBarrieredBase to TraceEdge and ReadBarrieredBase to IsAboutToBeFinalized, etc. To implement post-barriers for ReadBarriered we may need to duplicate some code, but I don't think it will be so bad.

The first patch in the series splits BarrieredBase from WriteBarrieredBase.
Attachment #8667496 - Attachment is obsolete: true
Attachment #8668611 - Flags: review?(jcoppeard)
(Assignee)

Comment 11

3 years ago
Created attachment 8668612 [details] [diff] [review]
2_common_storage_with_ReadBarriered-v0.diff

And for ReadBarriered.
Attachment #8668612 - Flags: review?(jcoppeard)
(Assignee)

Comment 12

3 years ago
Created attachment 8668646 [details] [diff] [review]
3_share_tracing_accessor-v0.diff

Stop cargo-culting unsafeGet. We have a const and a non-const version and I was wondering if we could get rid of the non-const unsafeGet to plug one hole where barriers could escape. Turns out this is used exclusively by tracing, so I've renamed it as appropriate and removed the other unsafeGet. It is now quite clear that all unbarriered, indirect barrier usage happens as a direct result of tracing.
Attachment #8668646 - Flags: review?(jcoppeard)
(Assignee)

Comment 13

3 years ago
After having done all of the above so that we can have TenuredReadBarriered and apply it in the two places where it could possibly matter for RegExp, our new performance is: even slower, by roughly the same amount again (100pts).

As this doesn't appear to affect windows or 64bit macos, I'm inclined to just land everything above and sign off on the "regression."
(Assignee)

Comment 14

3 years ago
Created attachment 8668714 [details] [diff] [review]
4_add_postbarrier_to_readbarriered-v0.diff

The original patch, rebased.
Attachment #8668714 - Flags: review?(jcoppeard)
Comment on attachment 8668611 [details] [diff] [review]
1_rename_BarrieredBase_to_WriteBarrieredBase-v0.diff

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

This all looks good.

::: js/src/gc/Barrier.cpp
@@ +29,5 @@
> +}
> +#define INSTANTIATE_ALL_VALID_TYPES(type) \
> +    template void js::BarrieredBase<type>::assertTypeConstraints() const;
> +FOR_EACH_GC_POINTER_TYPE(INSTANTIATE_ALL_VALID_TYPES)
> +#undef INSTANTIATE_ALL_VALID_TYPES

I was wondering why this was out-of-line but the I saw the comment in the header.  Unfortunate because I think we will get a link failure rather than seeing these errors if the class is used with an inappropriate type.
Attachment #8668611 - Flags: review?(jcoppeard) → review+
Comment on attachment 8668612 [details] [diff] [review]
2_common_storage_with_ReadBarriered-v0.diff

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

Since classes derived from ReadBarrieredBase are going to get write barriers too, would it make more sense to name these two base classes after the fact that they wrap strong/weak references rather than the type of barriers they use?  It would make more sense in the marking interface so show that we only support tracing strong references.
Attachment #8668612 - Flags: review?(jcoppeard) → review+
Comment on attachment 8668646 [details] [diff] [review]
3_share_tracing_accessor-v0.diff

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

::: js/src/vm/ScopeObject.cpp
@@ +2103,5 @@
>       * missingScopes points to debug scopes weakly so that debug scopes can be
>       * released more eagerly.
>       */
>      for (MissingScopeMap::Enum e(missingScopes); !e.empty(); e.popFront()) {
> +        if (IsAboutToBeFinalized(&e.front().value())) {

Nice!
Attachment #8668646 - Flags: review?(jcoppeard) → review+

Updated

3 years ago
Attachment #8668714 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 18

3 years ago
(In reply to Jon Coppeard (:jonco) from comment #15)
> Comment on attachment 8668611 [details] [diff] [review]
> 1_rename_BarrieredBase_to_WriteBarrieredBase-v0.diff
> 
> Review of attachment 8668611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This all looks good.
> 
> ::: js/src/gc/Barrier.cpp
> @@ +29,5 @@
> > +}
> > +#define INSTANTIATE_ALL_VALID_TYPES(type) \
> > +    template void js::BarrieredBase<type>::assertTypeConstraints() const;
> > +FOR_EACH_GC_POINTER_TYPE(INSTANTIATE_ALL_VALID_TYPES)
> > +#undef INSTANTIATE_ALL_VALID_TYPES
> 
> I was wondering why this was out-of-line but the I saw the comment in the
> header.  Unfortunate because I think we will get a link failure rather than
> seeing these errors if the class is used with an inappropriate type.

You are correct. Unfortunately, Barrier.h is used basically everywhere, so we'd need to introduce a new header for FOR_EACH_GC_POINTER_TYPE. Not the end of the world, but I wound up going this route as it's a fairly unlikely error and our headers are already have a bit of a mess.

(In reply to Jon Coppeard (:jonco) from comment #16)
> Comment on attachment 8668612 [details] [diff] [review]
> 2_common_storage_with_ReadBarriered-v0.diff
> 
> Review of attachment 8668612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since classes derived from ReadBarrieredBase are going to get write barriers
> too, would it make more sense to name these two base classes after the fact
> that they wrap strong/weak references rather than the type of barriers they
> use?  It would make more sense in the marking interface so show that we only
> support tracing strong references.

Yes indeed! That is actually my intention. Unfortunately, it's not at all clear to me that that naming reflects reality at the moment, at least on the weak-ref side. My plan is to create a WeakRef type based on ReadBarriered that automatically sweeps the ref weakly, convert everything over piecemeal, then fix the names to be proscriptive rather than descriptive.

Comment 20

3 years ago
This time there is no regression on AWFY :)
(Assignee)

Comment 21

3 years ago
Well, the code I landed ended up landing is identical to the code that landed initially. So glad I spent a day of my life addressing this.
https://hg.mozilla.org/mozilla-central/rev/af829895379d
https://hg.mozilla.org/mozilla-central/rev/b297bcfe0504
https://hg.mozilla.org/mozilla-central/rev/d36103a859ac
https://hg.mozilla.org/mozilla-central/rev/02e6762578c6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.