Closed Bug 1372524 Opened 3 years ago Closed 3 years ago

Refactor weak cache implementation

Categories

(Core :: JavaScript: GC, enhancement)

55 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

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())
Attachment #8877079 - Flags: review?(sphink)
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
Attachment #8877079 - Flags: review?(sphink) → review+
(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 :)
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1568e838bd5
Refactor WeakCache implementation a little r=sfink
You need to log in before you can comment on or make changes to this bug.