1372524 - Refactor weak cache implementation

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

Attachment: refactor-weak-cache

In preparation for bug 1367795, here's a patch to refactor the weak cache implementation.  It does the following:
 - use a base class and virtual dispatch for sweeping rather than storing a function pointer (removes casting to WeakCache<void*>)
 - move the WeakCache into the set type (removes some .get() calls)
 - allow any number of arguments for the constructor (removes construction with Set())

Comment 1

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8877079 [details] [diff] [review]
refactor-weak-cache

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

Very nice! Using a vtable makes so much more sense.

::: js/public/SweepingAPI.h
@@ +22,3 @@
>  } // namespace shadow
>  
> +namespace detail {

This is the only thing I'm not sure about. It seems like WeakCacheBase is part of the public API for using these things. So should it be in detail? I guess I thought of detail as being for things that are internal and not exposed to any users, at least for the most part.

(But it's not a big deal either way.)

@@ +27,5 @@
> +    WeakCacheBase() = delete;
> +    WeakCacheBase(const WeakCacheBase&) = delete;
> +
> +  public:
> +    WeakCacheBase(Zone* zone) {

explicit

@@ +30,5 @@
> +  public:
> +    WeakCacheBase(Zone* zone) {
> +        shadow::RegisterWeakCache(zone, this);
> +    }
> +    WeakCacheBase(JSRuntime* rt) {

explicit

@@ +54,5 @@
>    public:
>      using Type = T;
>  
> +    template <typename... Args>
> +    WeakCache(Zone* zone, Args&&... args)

explicit

@@ +58,5 @@
> +    WeakCache(Zone* zone, Args&&... args)
> +      : WeakCacheBase(zone), cache(mozilla::Forward<Args>(args)...)
> +    {}
> +    template <typename... Args>
> +    WeakCache(JSRuntime* rt, Args&&... args)

explicit

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)
Clients don't need to use the base class directly so I feel it's an internal thing and should live in detail.  But I'm not 100% clear on exactly what the rules are for using the detail namespace.  I'll leave it like it is for now but I'm happy to change it later.

Thanks for pointing out the missing 'explict's and saving me from breaking inbound :)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1568e838bd5
Refactor WeakCache implementation a little r=sfink

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2599d6d9a4b1
Add missing explicit keywords r=me

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e1568e838bd5
https://hg.mozilla.org/mozilla-central/rev/2599d6d9a4b1