Closed Bug 1118686 Opened 7 years ago Closed 7 years ago

TraceLoggerGraph: add upper bound before flushing.


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox35 --- unaffected
firefox36 --- unaffected
firefox37 --- fixed
firefox38 --- fixed


(Reporter: h4writer, Assigned: h4writer)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → hv1989
Attachment #8545159 - Flags: review?(benj)
Blocks: 1065722
Comment on attachment 8545159 [details] [diff] [review]

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

r+ with requested changes

::: js/src/vm/TraceLoggingGraph.cpp
@@ +102,5 @@
>          }
>      }
>      int written = fprintf(out, "{\"tree\":\"\", \"events\":\"\", "
> +                               "\"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.
Attachment #8545159 - Flags: review?(benj) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attached patch PatchSplinter Review
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]:
Attachment #8545159 - Attachment is obsolete: true
Attachment #8549462 - Flags: review+
Attachment #8549462 - Flags: approval-mozilla-aurora?
Comment on attachment 8549462 [details] [diff] [review]

Attachment #8549462 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.