Closed Bug 1225298 Opened 9 years ago Closed 8 years ago

Use GC policy mechanism for sweeping hashtable-based collections

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(14 files, 5 obsolete files)

29.12 KB, patch
terrence
: review+
sfink
: checkin+
Details | Diff | Splinter Review
6.15 KB, patch
terrence
: review+
Details | Diff | Splinter Review
3.56 KB, patch
terrence
: review+
sfink
: checkin+
Details | Diff | Splinter Review
4.86 KB, patch
terrence
: review+
Details | Diff | Splinter Review
13.82 KB, patch
terrence
: review+
Details | Diff | Splinter Review
7.29 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.02 KB, patch
terrence
: review+
sfink
: checkin+
Details | Diff | Splinter Review
3.52 KB, patch
terrence
: review+
sfink
: checkin+
Details | Diff | Splinter Review
6.62 KB, patch
terrence
: review+
sfink
: checkin+
Details | Diff | Splinter Review
5.16 KB, patch
terrence
: review+
sfink
: checkin+
Details | Diff | Splinter Review
4.22 KB, patch
terrence
: review+
sfink
: checkin+
Details | Diff | Splinter Review
5.01 KB, patch
terrence
: review+
Details | Diff | Splinter Review
5.25 KB, patch
terrence
: review+
Details | Diff | Splinter Review
4.20 KB, patch
Details | Diff | Splinter Review
TraceableHashMap and friends have this nice GCPolicy thing. It would be good to use it for sweeping too.
Alrighty, time to start on the review cycle. There's some questionable naming in here -- I removed the "Policy" suffix on some things, and perhaps shouldn't have.

This broadens TraceableHashTable to be usable for both tracing and sweeping. It's a little odd, in that it only inherits from Traceable, and it depends on having an appropriate GCPolicy to decide whether it's really traceable, sweepable, or both.

I'm also going to attach some patches converting stuff over, since it's impossible to evaluate this stuff without seeing it used. Sadly, so far sweeping *always* requires custom code. These are a somewhat random assortment of 4 tables that looked like they might be useful for hashing out the necessary API.

Oh, right. More questionable naming: this patch calls the main entry point sweepEntry rather than isAboutToBeFinalized, partly because iABtF is a mouthful and partly because I think of entries as getting "swept" away more than "finalized". The various data that the sweeping decision depends on do kinda get finalized, but the decision for the entry is a function of those data. Now I'm feeling like maybe I'm making a pointless break with existing terminology, though. Whaddaya think?

Also, I hope the Rekey stuff will go away eventually. That may enable some more simplification and defaulting of the policy API, since it really tangles things up by depending on the TableType::Enum typename (which is a bit of a circular dependency).
Attachment #8688098 - Flags: review?(terrence)
Use GCHashMap for CrossCompartmentWrapperMap. A pretty straightforward case.
Attachment #8688099 - Flags: review?(terrence)
Attached patch Use GCHashSet for atoms table (obsolete) — Splinter Review
Now the atoms table, which required a bit of weirdness. The ordering of definitions, in particular, is a little grotty because the Lookup type is not the same as the Key type. So I have to define a Rekey thing that knows how to build a Lookup for a key.
Attachment #8688101 - Flags: review?(terrence)
Attached patch Use GCHashSet for BaseShapeSet (obsolete) — Splinter Review
Shapes are always fun. Still not finding a lot of commonality to put into a DefaultSomething.
Attachment #8688102 - Flags: review?(terrence)
Comment on attachment 8688098 [details] [diff] [review]
Use GC policy mechanism for sweeping hashtable-based collections

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

This looks mostly good, but I feel like there's still a fairly low-level API mismatch here, even if it's mostly in the naming.

Specifically, I see GCPolicy<T> as working on directed Edges in the graph, but producing and consuming information about the target of the edge.

So where we have |void GCPolicy<T>::trace(T)| producing a mark, I see |bool GCPolicy<T>::[name needed](T*) as returning the mark (or some function of the mark). Regarding IsAboutToBeFinalized, remember that we have a 3-phase GC: mark, sweep, finalize. I read this as "the target of this pointer will be dead at the start of the /next/ phase (and maybe you'd like to do something about that)". At one point I tried to change all of the IAtbF to !IsMarked. I think Jon eventually had to change most of them back because incremental sweeping doesn't want the mark bit exactly, it wants a function of the mark bit and the current GC state.

All of which should explain why I dislike the name |sweepEdge|: it's both too verby and tied too closely to the marking tracer. Note, this also explains why I hate the IsAboutToBeFinalized name, entirely aside from it's wordyness: the name is about the target of the edge, whereas the question it's answering is (or at least should be!) all about the edge itself -- Is this edge going to be unsafe to access when we next run the mutator? Let's discuss some of the following potential names in the GC meeting tomorrow morning: isValid; isDead; isSweepable; wasTraced; needsSweep; and probably many more.

And this should go some way to explain the API mismatch that I'm seeing: the weak GCHashMap edges are edges with some aggregate lifetime that is some [static] function of the [dynamic] lifetimes of the key and value. i.e. GCMapPolicy is not fundamentally different from GCPolicy and it should have a much closer API than it has here. Moreover, given the wide spread of potential implementations, I think it might make the most sense to simply not have DefaultMapGCPolicy define a default policy for sweeping at all. That said, we probably do need some policy so that things will compile, so that policy should almost certainly be GCPolicy<Key>::needSweep(k) || GCPolicy<Value>::needSweep(v) or somesuch. I'll try to explain further inline.

Also, let's just totally drop all the rekey logic. It's almost all gone on tip and converting things over to stable hash codes is more or less trivial now anyway.

::: js/public/GCHashTable.h
@@ +16,5 @@
>  // Define a reasonable default GC policy for GC-aware Maps.
>  template <typename Key, typename Value>
>  struct DefaultMapGCPolicy {
> +    using TraceKey = DefaultGCPolicy<Key>;
> +    using TraceValue = DefaultGCPolicy<Value>;

I think it still makes sense to have KeyPolicy and ValuePolicy -- those sub-structures tell us how to trace or needSweep a key or edge individually. We may not need to do needSweep individually, but we do need trace individually, so I think that this really does want to remain as KeyPolicy and ValuePolicy.

@@ +17,5 @@
>  template <typename Key, typename Value>
>  struct DefaultMapGCPolicy {
> +    using TraceKey = DefaultGCPolicy<Key>;
> +    using TraceValue = DefaultGCPolicy<Value>;
> +    using SweepEntry = DefaultGCMapSweepPolicy<Key, Value>;

Where Key,ValueTrace define |trace| and |needSweep| for edge T, this structure itself wants to define |trace| and |needSweep| for a table entry (edge from the container to the... contained... stuff). I didn't create an aggregate |trace| function initially because I am lazy (and the complexity of rekey), not because it was a good idea. I think this structure should look more like:

template <typename Key, typename Value>
struct DefaultMapGCPolicy {
    using KeyPolicy = DefaultGCPolicy<Key>;
    using ValuePolicy = DefaultGCPolicy<Value>;

    static void trace(JSTracer* trc, Key* key, Value* value, const char* name) {
        KeyPolicy::trace(trc, key, name);
        ValuePolicy::trace(trc, value, name);
    }

    static bool needSweep(Key* key, Value* value) {
        return KeyPolicy::needSweep(key) || ValuePolicy::needSweep(value);
    }
};

@@ +184,5 @@
> +// Define a reasonable default GC policy for GC-aware Sets.
> +template <typename Value, typename Enum>
> +struct DefaultGCSetPolicy {
> +    using TraceEntry = DefaultGCPolicy<Value>;
> +    using SweepEntry = DefaultGCSetSweepPolicy<Value>;

I don't think we need or want policy on the set itself. If the user wants to keep/reject an entry with something more complicated than IsAboutToBeFinalized(&e.front()), then they are free to craft a |struct MyGCPolicy { static bool needsRekey(JSWhatev* w) { return w->myComplexConditions(); }|. Even if every user just passes through trace with using trace = DefaultTracer<JSWhatev>::trace, I don't think that's a major loss.

::: js/public/TracingAPI.h
@@ +395,5 @@
> +        // implementation of DefaultGCEntryPolicy<K,V> for your type or, if this is
> +        // the right policy for you, your struct or container is missing a
> +        // sweep method.
> +        t->sweep();
> +    }

This is the other major confusion I'm seeing. JS::Traceable provides |void trace(JSTracer*)| and causes us to trace ourself. GCPolicy<T> provides |void trace(JSTracer*, T*, const char*)|, which traces edges to T*, not the T itself. This function belongs in JS::Sweepable. What we want here is |static bool needSweep(T* t)|.

@@ +401,5 @@
>  
>  // This policy ignores any GC interaction, e.g. for non-GC types.
>  template <typename T>
>  struct IgnoreGCPolicy {
>      static void trace(JSTracer* trc, uint32_t* id, const char* name) {}

I think we just want to add a |static bool needsSweep(T*)|.

@@ +406,5 @@
>  };
> +template <typename Key, typename Value>
> +struct IgnoreGCSweepPolicy {
> +    static bool sweepEntry(Key* k, Value* v, bool *needsRekey) { return false; }
> +};

I don't think we want to define any defaults for aggregate edges, so let's skip this until we have at least 2 users that want it.

@@ +410,5 @@
> +};
> +template <typename Value>
> +struct IgnoreGCSetSweepPolicy {
> +    static bool sweepEntry(Value* v, bool *needsRekey) { return false; }
> +};

I can't think of any instances where it makes sense to only define tracing or sweeping, but not both, so I think IgnoreGCSetSweepPolicy is just IgnoreGCPolicy. Please remove this.

@@ +430,5 @@
> +struct DefaultGCSetRekeyPolicy {
> +    static void rekeyFront(Enum& e, Value v) {
> +        e.rekeyFront(v);
> +    }
> +};

Kill. Use fire. This may help: http://fringe.davesource.com/Fringe/Explosives/Ignition.An-Informal-History-of-Liquid-Rocket-Propellants.John-D-Clark.pdf

@@ +437,5 @@
> +struct GCSetNoRekeyPolicy {
> +    static void rekeyFront(Enum& e, Value v) {
> +        MOZ_CRASH("rekey called using a no-rekey policy. sweepEntry should not be setting needsRekey.");
> +    }
> +};

Kill.
Attachment #8688098 - Flags: review?(terrence)
(In reply to Terrence Cole [:terrence] from comment #5)
> Comment on attachment 8688098 [details] [diff] [review]
> Use GC policy mechanism for sweeping hashtable-based collections
> 
> Review of attachment 8688098 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks mostly good, but I feel like there's still a fairly low-level API
> mismatch here, even if it's mostly in the naming.
> 
> Specifically, I see GCPolicy<T> as working on directed Edges in the graph,
> but producing and consuming information about the target of the edge.
> 
> So where we have |void GCPolicy<T>::trace(T)| producing a mark, I see |bool
> GCPolicy<T>::[name needed](T*) as returning the mark (or some function of
> the mark). Regarding IsAboutToBeFinalized, remember that we have a 3-phase
> GC: mark, sweep, finalize. I read this as "the target of this pointer will
> be dead at the start of the /next/ phase (and maybe you'd like to do
> something about that)". At one point I tried to change all of the IAtbF to
> !IsMarked. I think Jon eventually had to change most of them back because
> incremental sweeping doesn't want the mark bit exactly, it wants a function
> of the mark bit and the current GC state.

That's fair.

Some of this is because I chose the cross-compartment wrapper map as my first example, and it chooses whether to discard an entry in the table based on a function of both the key and value. So I started thinking in terms of table entries, because I was paranoid about other things needing more complex stuff. "Can we throw this entry away? I'll give you both the key and the value to help make the decision." But you're right; even this case can separate out the key and the value, and that means we don't need to think of the entry as a unit that can be iAtbF or not.

Also, in my mind "sweep" is used for both scanning through the tables, and for throwing stuff out. So I read if (sweepEntry(...)) as "try to discard this entry. If you can, then...."

I like separating out the keys and values much better, though I'm still a little hung up on what the key check. Hm... I guess I mean the same thing as you, where it's really a function of the edge, not the target. The target itself can't tell you whether or not it should be thrown out of the table; that depends on the purpose of the specific table. You can think of that as an edge between the table and the target.

> All of which should explain why I dislike the name |sweepEdge|: it's both
> too verby and tied too closely to the marking tracer. Note, this also

Actually, it's sweepEntry. sweepEdge would be a somewhat better name. :-)

> explains why I hate the IsAboutToBeFinalized name, entirely aside from it's
> wordyness: the name is about the target of the edge, whereas the question
> it's answering is (or at least should be!) all about the edge itself -- Is
> this edge going to be unsafe to access when we next run the mutator? Let's

Yeah, that's where I was getting hung up too. It's not really asking iAtbF(key), it's really asking iAtbF(edge) or iAtbF(table, key) or shouldDiscard(entry).

> discuss some of the following potential names in the GC meeting tomorrow
> morning: isValid; isDead; isSweepable; wasTraced; needsSweep; and probably
> many more.
> 
> And this should go some way to explain the API mismatch that I'm seeing: the
> weak GCHashMap edges are edges with some aggregate lifetime that is some
> [static] function of the [dynamic] lifetimes of the key and value. i.e.
> GCMapPolicy is not fundamentally different from GCPolicy and it should have
> a much closer API than it has here. Moreover, given the wide spread of
> potential implementations, I think it might make the most sense to simply
> not have DefaultMapGCPolicy define a default policy for sweeping at all.
> That said, we probably do need some policy so that things will compile, so
> that policy should almost certainly be GCPolicy<Key>::needSweep(k) ||
> GCPolicy<Value>::needSweep(v) or somesuch. I'll try to explain further
> inline.

Right, if I change it to be based on edge-to-key and edge-to-value, that makes total sense.

> Also, let's just totally drop all the rekey logic. It's almost all gone on
> tip and converting things over to stable hash codes is more or less trivial
> now anyway.

\o/

> ::: js/public/GCHashTable.h
> @@ +16,5 @@
> >  // Define a reasonable default GC policy for GC-aware Maps.
> >  template <typename Key, typename Value>
> >  struct DefaultMapGCPolicy {
> > +    using TraceKey = DefaultGCPolicy<Key>;
> > +    using TraceValue = DefaultGCPolicy<Value>;
> 
> I think it still makes sense to have KeyPolicy and ValuePolicy -- those
> sub-structures tell us how to trace or needSweep a key or edge individually.
> We may not need to do needSweep individually, but we do need trace
> individually, so I think that this really does want to remain as KeyPolicy
> and ValuePolicy.

Yep. While Rekey was around and needed something else, there wasn't that much gain to focusing everything on keys vs values.

> @@ +17,5 @@
> >  template <typename Key, typename Value>
> >  struct DefaultMapGCPolicy {
> > +    using TraceKey = DefaultGCPolicy<Key>;
> > +    using TraceValue = DefaultGCPolicy<Value>;
> > +    using SweepEntry = DefaultGCMapSweepPolicy<Key, Value>;
> 
> Where Key,ValueTrace define |trace| and |needSweep| for edge T, this
> structure itself wants to define |trace| and |needSweep| for a table entry
> (edge from the container to the... contained... stuff). I didn't create an
> aggregate |trace| function initially because I am lazy (and the complexity
> of rekey), not because it was a good idea. I think this structure should
> look more like:
> 
> template <typename Key, typename Value>
> struct DefaultMapGCPolicy {
>     using KeyPolicy = DefaultGCPolicy<Key>;
>     using ValuePolicy = DefaultGCPolicy<Value>;
> 
>     static void trace(JSTracer* trc, Key* key, Value* value, const char*
> name) {
>         KeyPolicy::trace(trc, key, name);
>         ValuePolicy::trace(trc, value, name);
>     }
> 
>     static bool needSweep(Key* key, Value* value) {
>         return KeyPolicy::needSweep(key) || ValuePolicy::needSweep(value);
>     }
> };

Yep.

> @@ +184,5 @@
> > +// Define a reasonable default GC policy for GC-aware Sets.
> > +template <typename Value, typename Enum>
> > +struct DefaultGCSetPolicy {
> > +    using TraceEntry = DefaultGCPolicy<Value>;
> > +    using SweepEntry = DefaultGCSetSweepPolicy<Value>;
> 
> I don't think we need or want policy on the set itself. If the user wants to
> keep/reject an entry with something more complicated than
> IsAboutToBeFinalized(&e.front()), then they are free to craft a |struct
> MyGCPolicy { static bool needsRekey(JSWhatev* w) { return
> w->myComplexConditions(); }|. Even if every user just passes through trace
> with using trace = DefaultTracer<JSWhatev>::trace, I don't think that's a
> major loss.

Yeah, ok. That kind of falls out of the above.

> 
> ::: js/public/TracingAPI.h
> @@ +395,5 @@
> > +        // implementation of DefaultGCEntryPolicy<K,V> for your type or, if this is
> > +        // the right policy for you, your struct or container is missing a
> > +        // sweep method.
> > +        t->sweep();
> > +    }
> 
> This is the other major confusion I'm seeing. JS::Traceable provides |void
> trace(JSTracer*)| and causes us to trace ourself. GCPolicy<T> provides |void
> trace(JSTracer*, T*, const char*)|, which traces edges to T*, not the T
> itself. This function belongs in JS::Sweepable. What we want here is |static
> bool needSweep(T* t)|.

Yeah, I didn't think too hard about this. It was early cut & paste, and then I left it because I thought it might end up being used for a PersistentSweepable-type thing, but really I didn't think about it.

> @@ +401,5 @@
> >  
> >  // This policy ignores any GC interaction, e.g. for non-GC types.
> >  template <typename T>
> >  struct IgnoreGCPolicy {
> >      static void trace(JSTracer* trc, uint32_t* id, const char* name) {}
> 
> I think we just want to add a |static bool needsSweep(T*)|.

Uh, yeah, I'll have to think about what's needed here anymore.

> @@ +406,5 @@
> >  };
> > +template <typename Key, typename Value>
> > +struct IgnoreGCSweepPolicy {
> > +    static bool sweepEntry(Key* k, Value* v, bool *needsRekey) { return false; }
> > +};
> 
> I don't think we want to define any defaults for aggregate edges, so let's
> skip this until we have at least 2 users that want it.

Oh. This was because I expected the common case to be tables with strong edges to their keys & values, so they didn't need to be swept. Which is kind of awful, because then I'd be relying on the compiler to elide an empty sweep loop.

> @@ +410,5 @@
> > +};
> > +template <typename Value>
> > +struct IgnoreGCSetSweepPolicy {
> > +    static bool sweepEntry(Value* v, bool *needsRekey) { return false; }
> > +};
> 
> I can't think of any instances where it makes sense to only define tracing
> or sweeping, but not both, so I think IgnoreGCSetSweepPolicy is just
> IgnoreGCPolicy. Please remove this.
> 
> @@ +430,5 @@
> > +struct DefaultGCSetRekeyPolicy {
> > +    static void rekeyFront(Enum& e, Value v) {
> > +        e.rekeyFront(v);
> > +    }
> > +};
> 
> Kill. Use fire. This may help:
> http://fringe.davesource.com/Fringe/Explosives/Ignition.An-Informal-History-
> of-Liquid-Rocket-Propellants.John-D-Clark.pdf

I knew somebody growing up who had a hobby of creating fairly large (eg 25 feet long) liquid fuel rockets. He's still alive, afaik.
Ok, how does this look? I've updated the WrapperMap and atoms table patches for it, and it looks ok.
Attachment #8688260 - Flags: review?(terrence)
Attachment #8688098 - Attachment is obsolete: true
That's better, but I feel like I may not have expressed the core observation forcefully enough. Please take a closer look at the defaul GCPolicy I wrote for Map. It's a "normal" GCPolicy but it has a different interface; specifically one that makes for Maps and not one that is identical to GCPolicy<some-random-T>. Once we have that, the Maps sweep should just be doing:

for(Entry e : all())
    if (GCPolicy::needSweep(&e.key(), &e.value()))
        e.removeFront();

Then the Map's *Default*MapGCPolicy will do the iAtbF(key)||iAtbF(value) what you have in sweep by /default/, but still be totally overridable by users that need different behavior.
Ok, I've gone through almost all of the relevant tables now, and haven't needed to add anything beyond this version if I ignore the things I gave up on. And for those, I think it'd be better to do in a later pass.
Attachment #8689254 - Flags: review?(terrence)
Attachment #8688260 - Attachment is obsolete: true
Attachment #8688260 - Flags: review?(terrence)
This removes a rekey, and so is dependent on bug 1225577.
Attachment #8689257 - Flags: review?(terrence)
Attachment #8688099 - Attachment is obsolete: true
Attachment #8688099 - Flags: review?(terrence)
No rekeying. Ready to go.
Attachment #8689258 - Flags: review?(terrence)
Attachment #8688101 - Attachment is obsolete: true
Attachment #8688101 - Flags: review?(terrence)
Murders a rekey. Dependent on bug 1224050 and removing rekeying from BaseShapeSet.
Attachment #8689260 - Flags: review?(terrence)
Attachment #8688102 - Attachment is obsolete: true
Attachment #8688102 - Flags: review?(terrence)
Removes a rekey. Depends on bug 1224038 as well as removing rekeying for AllocationSiteTable and ArrayObjectTable.
Attachment #8689263 - Flags: review?(terrence)
No rekeying here. Ready to go.
Attachment #8689264 - Flags: review?(terrence)
This is the best case sort of change. Just deletes code, really, since it relies on defaults.
Attachment #8689265 - Flags: review?(terrence)
Simplified things nicely.
Attachment #8689266 - Flags: review?(terrence)
I was terrified of this one, but it ended up avoiding all the hard stuff. It's unfortunate that this only handles major GC sweeping, though; there's still some duplicate code for other sweeps.
Attachment #8689268 - Flags: review?(terrence)
This only fixes one of two tables, because the missingScopes table requires access to stuff that we can't see here.
Attachment #8689269 - Flags: review?(terrence)
Depends on: 1224050, 1225577, 1224038
Comment on attachment 8689254 [details] [diff] [review]
Use GC policy mechanism for sweeping hashtable-based collections

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

Gorgeous!

::: js/public/GCHashTable.h
@@ +27,5 @@
> +// A GCHashMap is a GC-aware HashMap, meaning that it has additional trace and
> +// sweep methods that know how to visit all keys and values in the table.
> +// HashMaps that contain GC pointers will generally want to use this GCHashMap
> +// specialization in lieu of HashMap, either because those pointer must be
> +// traced to be kept alive -- in which case, TraceKey and/or TraceValue

TraceKey and TraceValue are out of date names.

@@ +29,5 @@
> +// HashMaps that contain GC pointers will generally want to use this GCHashMap
> +// specialization in lieu of HashMap, either because those pointer must be
> +// traced to be kept alive -- in which case, TraceKey and/or TraceValue
> +// should do the appropriate tracing -- or because those pointers are weak and
> +// must be swept during a GC -- in which case SweepEntry should be set

Ditto SweepEntry.
Attachment #8689254 - Flags: review?(terrence) → review+
Comment on attachment 8689269 [details] [diff] [review]
Use GCHashMap for liveScopes

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

\o/
Attachment #8689269 - Flags: review?(terrence) → review+
Attachment #8689266 - Flags: review?(terrence) → review+
Comment on attachment 8689258 [details] [diff] [review]
Use GCHashSet for atoms table

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

\o/
Attachment #8689258 - Flags: review?(terrence) → review+
Mindless conversion of UniqueIdMap. Also includes a pointless specialization of DefaultGCPolicy for uint64_t (it's not used in this patch). Though it would be if I defined a DefaultGCPolicy<Cell*> instead of making a UniqueIdGCPolicy, but that seems a little too general?
Attachment #8689282 - Flags: review?(terrence)
Umm... ok, I don't understand this one. It looks to me like WeakCache is completely redundant now, which is not too surprising. But I don't see how functionWrappers_ is ever swept currently. I can't find any calls to its sweep() method.

I guess I'll try setting a gdb breakpoint.
Attachment #8689291 - Flags: review?(terrence)
(In reply to Steve Fink [:sfink, :s:] from comment #23)
> Created attachment 8689291 [details] [diff] [review]
> Use GCHashMap for VMWrapperMap
> 
> Umm... ok, I don't understand this one. It looks to me like WeakCache is
> completely redundant now, which is not too surprising. But I don't see how
> functionWrappers_ is ever swept currently. I can't find any calls to its
> sweep() method.
> 
> I guess I'll try setting a gdb breakpoint.

Well, I added A MOZ_CRASH and ran octane. It isn't called. jandem or Terrence, do you know the story here? Is this just a leak or what?
Flags: needinfo?(jdemooij)
Oh. Maybe the whole table is nuked periodically instead? Sadly, I'm out of battery on both my laptop and very soon my phone. I will check tomorrow.
(In reply to Steve Fink [:sfink, :s:] from comment #24)
> It isn't called. jandem or Terrence, do you know the story here?

JitCodes for trampolines and function wrappers are stored in the atoms compartment when initializing the JitRuntime, and are never swept/collected, so AFAIK this is by design. They used to be per-compartment and compiled-on-demand, but they were moved to the runtime/atoms-compartment to simplify off-thread compilation. It's possible WeakCache is no longer the best data structure for this, as it's not really a weak reference.
Flags: needinfo?(jdemooij)
Are there a fixed number of function trampolines? I guess even if there are, there are probably enough to warrant some machinery to sweep it safely, particularly if we can re-use the infrastructure.
Attachment #8689264 - Flags: review?(terrence) → review+
Attachment #8689265 - Flags: review?(terrence) → review+
Attachment #8689268 - Flags: review?(terrence) → review+
Comment on attachment 8689282 [details] [diff] [review]
Use GCHashMap for UniqueIdMap

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

Neat!
Attachment #8689282 - Flags: review?(terrence) → review+
Comment on attachment 8689291 [details] [diff] [review]
Use GCHashMap for VMWrapperMap

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

Wow, nice!

::: js/src/jsweakcache.h
@@ -69,5 @@
> -};
> -
> -} // namespace js
> -
> -#endif /* jsweakcache_h */

\o/
Attachment #8689291 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/b52488f1bad9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Depends on: 1226687
Depends on: 1226801
Oops, seems like I neglected to upload this patch for review. I know I won't get review for a while, but in my stack this is currently the only patch underneath the RegExpShared patch anyway, and that one is producing *weird* failures on try that'll take me some time to figure out. (Somehow, the main thread is proceeding before the RegExp sweeping thread is done.)
Attachment #8694328 - Flags: review?(terrence)
Attachment #8689258 - Flags: checkin+
Attachment #8689265 - Flags: checkin+
Attachment #8689266 - Flags: checkin+
Attachment #8689268 - Flags: checkin+
Attachment #8689269 - Flags: checkin+
Attachment #8689282 - Flags: checkin+
Attachment #8689254 - Flags: checkin+
Comment on attachment 8694328 [details] [diff] [review]
Use GCHashMap for ICStubCodeMap

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

Woot!

::: js/src/jit/JitCompartment.h
@@ +348,5 @@
>  class JitCompartment
>  {
>      friend class JitActivation;
>  
> +    struct IcStubCodeMapGCPolicy {

I think it should be capital IC, given that the type the policy is for is ICStubCodeMap.

::: js/src/jsweakcache.h
@@ -105,5 @@
> -            MOZ_ASSERT(ptr.found() && &*ptr == &r.front());
> -        }
> -#endif
> -    }
> -};

\o/
Attachment #8694328 - Flags: review?(terrence) → review+
Comment on attachment 8689260 [details] [diff] [review]
Use GCHashSet for BaseShapeSet and InitialShapeSet

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

I ran into a wall trying to fix the perf issues with rekey, so let's just land the working half first.
Attachment #8689260 - Flags: review?(terrence) → review+
Attachment #8689263 - Flags: review?(terrence) → review+
Attachment #8689257 - Flags: review?(terrence) → review+
Blocks: 1237445
Resolving this bug since we've had a release bump with some of the patches landed. Further landings will be in bug 1237445.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Here's a minimal change that brings RegExpShared a little more in line with the GCHashMap stuff, but doesn't actually go through GCHashMap. We can land it or not, it doesn't really matter either way.
Attachment #8709588 - Flags: review?(terrence)
Comment on attachment 8709588 [details] [diff] [review]
Use GCHashSet for RegExpShared sweeping

oops, wrong bug
Attachment #8709588 - Flags: review?(terrence)
You need to log in before you can comment on or make changes to this bug.