Closed Bug 1189490 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 8 obsolete files)

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+
Attachment #8641279 - Attachment is obsolete: true
Attachment #8645256 - Flags: review+
You need to log in before you can comment on or make changes to this bug.