1118686 - TraceLoggerGraph: add upper bound before flushing.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

The TraceLoggerGraph will increase its memory use whenever possible before flushing. Now this interacts badly with swap partitions, because using swap partitions the TraceloggerGraph can increase the memory well beyond the actual RAM size and swapping is bad for performance.

So add an upper bound to how large the tree can get before flushing. I went for 100mb as maximum. In normal files that means around 20 flush events, which is fine. But most importantly no swapping should happen with 100mb RAM usage.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8545159 [details] [diff] [review]
Patch

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

r+ with requested changes

::: js/src/vm/TraceLoggingGraph.cpp
@@ +102,5 @@
>          }
>      }
>  
>      int written = fprintf(out, "{\"tree\":\"tl-tree.%d.tl\", \"events\":\"tl-event.%d.tl\", "
> +                               "\"dict\":\"tl-dict.%d.json\", \"treeFormat\":\"64,64,31,1,32,v2\"}",

Do you really need this here?

@@ +296,5 @@
>  
>      if (!tree.hasSpaceForAdd()) {
> +        // If tree capacity is higher than around 100mb or the tree cannot get
> +        // increased in size, flush the content to disk.
> +        if (tree.capacity() > 1024*1024*5 ||

Could you make RHS a constant in the header files, please? In your comment, please be precise: do you mean 100 MB (mega-bytes) or 100 Mb (mega-bits)?

Also, it might be nice to make it explicit that sizeof(TreeEntry) == 24 bytes (and even static_assert this next to the computation of the constant, to make sure it gets changed if we ever change the TreeEntry structure), and that 5 * 24 ~= 100 Mb, making the link explicit between this 5 in the constant and the 100 MB in the comment.

Comment 3

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

https://hg.mozilla.org/mozilla-central/rev/c3ced50b2827

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1071546

[User impact if declined]:
Not much impact for regular users. It might annoy few people that they need to wait another release before tracelogger is working well. This just missed the merge, so a bit of a bummer.

[Describe test coverage new/current, TBPL]:
1 day on Inbound.

[Risks and why]: 
No risk. Small patch and only used by select public

[String/UUID change made/needed]:
/

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8549462 [details] [diff] [review]
Patch

Aurora+

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/871746328014