1209704 - Add a post barrier to ReadBarriered

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 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

Comment 1

[Terrence Cole [:terrence]]

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

Try run is looking good, let's do this!

Comment 2

[Jon Coppeard (:jonco)]

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?

Comment 3

[Terrence Cole [:terrence]]

(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 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1ab8950c41

Comment 5

[Guilherme Lima]

Apparently this caused a small regression on a few tests from Kraken and a bigger regression on Octane-RegExp.

Comment 6

[Terrence Cole [:terrence]]

So it did.

Comment 7

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d99aed4538

Comment 8

[Terrence Cole [:terrence]]

Oddly, I'm seeing a substantially higher score /with/ the patch applied locally.

Comment 9

[Terrence Cole [:terrence]]

And a small, but statistically relevant, bit slower on 32bit builds locally.

Comment 10

[Terrence Cole [:terrence]]

Attachment: 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.

Comment 11

[Terrence Cole [:terrence]]

Attachment: 2_common_storage_with_ReadBarriered-v0.diff

And for ReadBarriered.

Comment 12

[Terrence Cole [:terrence]]

Attachment: 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.

Comment 13

[Terrence Cole [:terrence]]

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."

Comment 14

[Terrence Cole [:terrence]]

Attachment: 4_add_postbarrier_to_readbarriered-v0.diff

The original patch, rebased.

Comment 15

[Jon Coppeard (:jonco)]

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.

Comment 16

[Jon Coppeard (:jonco)]

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.

Comment 17

[Jon Coppeard (:jonco)]

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!

Comment 18

[Terrence Cole [:terrence]]

(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 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af829895379d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b297bcfe0504
https://hg.mozilla.org/integration/mozilla-inbound/rev/d36103a859ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/02e6762578c6

Comment 20

[Guilherme Lima]

This time there is no regression on AWFY :)

Comment 21

[Terrence Cole [:terrence]]

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.

Comment 22

[Phil Ringnalda (:philor)]

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