Closed
Bug 1118686
Opened 10 years ago
Closed 10 years ago
TraceLoggerGraph: add upper bound before flushing.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox35 | --- | unaffected |
firefox36 | --- | unaffected |
firefox37 | --- | fixed |
firefox38 | --- | fixed |
People
(Reporter: h4writer, Assigned: h4writer)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
h4writer
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → hv1989
Attachment #8545159 -
Flags: review?(benj)
Comment 2•10 years ago
|
||
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.
Attachment #8545159 -
Flags: review?(benj) → review+
Assignee | ||
Updated•10 years ago
|
status-firefox37:
--- → affected
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 4•10 years ago
|
||
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?
Updated•10 years ago
|
Comment 5•10 years ago
|
||
Comment on attachment 8549462 [details] [diff] [review]
Patch
Aurora+
Attachment #8549462 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 6•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•