Closed Bug 1263772 Opened 8 years ago Closed 8 years ago

Use WeakCache to sweep the BaseShapes table

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

Steve did review for the infrastructure bits in bug 1233862 and bug 1263769, if you want some context.
Attachment #8740189 - Flags: review?(jcoppeard)
Comment on attachment 8740189 [details] [diff] [review]
weakcache_BaseShapeSet-v0.diff

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

::: js/src/jscompartment.cpp
@@ +1120,5 @@
>      *compartmentObject += mallocSizeOf(this);
>      objectGroups.addSizeOfExcludingThis(mallocSizeOf, tiAllocationSiteTables,
>                                          tiArrayTypeTables, tiObjectTypeTables,
>                                          compartmentTables);
> +    *compartmentTables += baseShapes.get().sizeOfExcludingThis(mallocSizeOf)

Maybe we can proxy sizeOfExcludingThis in GCHashMapOperations too.
Attachment #8740189 - Flags: review?(jcoppeard) → review+
Comment on attachment 8740189 [details] [diff] [review]
weakcache_BaseShapeSet-v0.diff

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

::: js/src/jscompartment.cpp
@@ +1120,5 @@
>      *compartmentObject += mallocSizeOf(this);
>      objectGroups.addSizeOfExcludingThis(mallocSizeOf, tiAllocationSiteTables,
>                                          tiArrayTypeTables, tiObjectTypeTables,
>                                          compartmentTables);
> +    *compartmentTables += baseShapes.get().sizeOfExcludingThis(mallocSizeOf)

Done.

::: js/src/vm/Shape.cpp
@@ +1292,5 @@
>  
>  /* static */ UnownedBaseShape*
>  BaseShape::getUnowned(ExclusiveContext* cx, StackBaseShape& base)
>  {
> +    BaseShapeSet& table = cx->compartment()->baseShapes.get();

We can use auto here to elide the type.

@@ +1299,5 @@
>          ReportOutOfMemory(cx);
>          return nullptr;
>      }
>  
>      DependentAddPtr<BaseShapeSet> p(cx, table, base);

And since the type itself is templated, we need to pass the real type here. This can be done as any of:
  1) DependentAddPtr<typename RemoveReference<decltype(table)>::Type> p(cx, table, base);
  2) DependentAddPtr<decltype(cx->compartment()->baseShapes)> p(cx, table, base);
  3) auto p = MakeDependentAddPtr(cx, table, base);

I opted for 3, e.g. wrapping the type signature behind a "make" function (as for make_unique or make_shared) and using decltype and typetraits internally to deduce the right template argument. This is pretty ugly, but it's much more limited scope than doing the same inline everywhere.

@@ +1355,5 @@
>  {
>      if (!baseShapes.initialized())
>          return;
>  
> +    for (BaseShapeSet::Enum e(baseShapes.get()); !e.empty(); e.popFront()) {

This one is a bit uglier. It's just a type mismatch: baseShapes is not a BaseShapeSet anymore. We can use .get(), or we can use decltype(baseShapes)::Enum. Not sure which I prefer. I've gone with the second for now as it stresses the Ops wrapper and I want that to be ready for prime-time.
https://hg.mozilla.org/mozilla-central/rev/86bd74d49e63
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.