[jsdbg2] The allocation and tenure promotions logs shouldn't use linked lists

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks 1 bug)

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(4 attachments, 8 obsolete attachments)

5.45 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
8.67 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
24.96 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
4.71 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
In broad strokes, here's the plan:

* Whip up an amortized O(1) FIFO queue backed by two vectors

* Make a DynamicTraceable version for gc/traceable things

* Use that instead of mozilla::LinkedList for the allocation and tenure promotion logs
Attachment #8641280 - Attachment is obsolete: true
Attachment #8641280 - Flags: review?(terrence)
Attachment #8641425 - Flags: review?(terrence)
This version of part 1 uses StaticTraceable instead of DynamicTraceable as we discussed on IRC.

I just ran some benchmarks (esprima JS parser parsing its own source while we use the Debugger API to sample allocations with P=.05) and didn't see any perf benefit that looks statistically significant. The benchmarks are fairly noisy with a pretty high stddev.

Without this patch series:

> [Stats total: 24.91024096679684s, mean: 0.04982048193359368s, stddev: 16%]

With this patch:

> [Stats total: 24.31464379882813s, mean: 0.04862928759765626s, stddev: 16%]

Pretty sure the stack capturing is way more expensive than the cache-unfriendly pointer following of the linked list traversal, but either way this actually simplifies the code quite nicely. Maybe it would show up if I was actually draining the logs in the benchmark...

(There's still some room to consolidate and abstract the common functionality of the logs in the future, but I will leave it for just that: the future.)
Comment on attachment 8641279 [details] [diff] [review]
Part 0: Add a FIFO queue container type to js/public

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

Nice!
Attachment #8641279 - Flags: review?(terrence) → review+
Attachment #8641425 - Flags: review?(terrence) → review+
Comment on attachment 8641426 [details] [diff] [review]
Part 2: Stop using mozilla::LinkedList for the allocations and tenure promotions logs and use js::TraceableFifo instead

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

Looks good.

::: js/src/vm/Debugger.cpp
@@ +1744,5 @@
> +    auto className = obj->getClass()->name;
> +    auto size = JS::ubi::Node(obj.get()).size(cx->runtime()->debuggerMallocSizeOf);
> +
> +    if (!allocationsLog.emplaceBack(wrappedFrame, when, className, ctorName, size))
> +    {

{ on the same line as the if ().
Attachment #8641426 - Flags: review?(terrence) → review+
You need to log in before you can comment on or make changes to this bug.