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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Add a FIFO queue container type to js/public

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add a DynamicTraceable version of the FIFO queue for use with GC things

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add a StaticTraceable version of the FIFO queue for use with GC things

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Stop using mozilla::LinkedList for the allocations and tenure promotions logs and use js::TraceableFifo instead

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 6

[Terrence Cole [:terrence]]

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!

Comment 7

[Terrence Cole [:terrence]]

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 ().

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cebec1f7c9db
https://hg.mozilla.org/integration/mozilla-inbound/rev/78f80496c70a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5158c4514c34
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0eef95cd0cb

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12371671&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/91cb27a1be1e

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Add a FIFO queue container type to js/public

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add a JS::Traceable version of the FIFO queue for use with GC things

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Stop using mozilla::LinkedList for the allocations and tenure promotions logs and use js::TraceableFifo instead

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Follow ups: Move [Traceable]Fifo to js/src/ds

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1183b20233c9

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/858b4f682c5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1ad27321de
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf626d3a647
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ec4867e4e8c

Comment 16

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/af32792fe260 for linux32 debug jit2 failures like https://treeherder.mozilla.org/logviewer.html#?job_id=12671345&repo=mozilla-inbound

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Add a FIFO queue container type to js/public

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add a JS::Traceable version of the FIFO queue for use with GC things

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Stop using mozilla::LinkedList for the allocations and tenure promotions logs and use js::TraceableFifo instead

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Follow ups: Move [Traceable]Fifo to js/src/ds

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Rebased and got a green try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4776797e693

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0477fe507c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0dddf4ef555
https://hg.mozilla.org/integration/mozilla-inbound/rev/069907acacef
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a68d5a3fdb6

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ad0477fe507c
https://hg.mozilla.org/mozilla-central/rev/b0dddf4ef555
https://hg.mozilla.org/mozilla-central/rev/069907acacef
https://hg.mozilla.org/mozilla-central/rev/5a68d5a3fdb6