Closed
Bug 1111564
Opened 9 years ago
Closed 9 years ago
2.35% Linux32 Tp5 Main RSS regression on inbound (v.37) Dec 11 from push c2659bf5793d
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jmaher, Assigned: Benjamin)
References
Details
(Keywords: perf, regression, Whiteboard: [talos_regression])
Here is a graph showing the regression: http://graphs.mozilla.org/graph.html#tests=%5B%5B261,131,33%5D%5D&sel=1418236292000,1418409092000&displayrange=7&datatype=running I did some retriggers on tbpl to verify this is really the cause: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=57725f8e7aa1&tochange=61ee2e053920&jobname=Ubuntu%20HW%2012.04%20mozilla-inbound%20talos%20tp5o This changeset the values go from ~154 -> ~157: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2659bf5793d
Reporter | ||
Comment 1•9 years ago
|
||
Benjamin, can you comment on if this is expected and if we should look into fixing this or accept it if the benefits outweigh the regression?
Flags: needinfo?(benjamin)
Assignee | ||
Comment 2•9 years ago
|
||
It's not unexpected in the sense we're trading a higher "basal" memory size for (many) less allocations and frees while running. Let's see what njn thinks.
Flags: needinfo?(benjamin)
Comment 3•9 years ago
|
||
How many compression threads do we expect to have alive at a time? Bug 1084177 comment 0 indicates that each compression instance uses 0.25 MiB of memory, so we'd need ~12 such threads to give a ~3 MiB regression.
Flags: needinfo?(benjamin)
Assignee | ||
Comment 4•9 years ago
|
||
There will be one zlib struct per JS vm helper thread. There's supposed to be one helper thread per CPU. Do the test boxes have 12 CPUs?
Flags: needinfo?(benjamin)
Comment 5•9 years ago
|
||
> Do the test boxes have 12 CPUs?
I'd be surprised if they did. So we should investigate if the number of helper threads is computed appropriately.
As for the patch: given that we have a clear RSS regression, and the benefit of avoiding the repeated allocs/deallocs is hard to quantify, I think we should back it out. It was a bad suggestion on my part, and I apologize for wasting your time, Benjamin :(
Assignee | ||
Comment 6•9 years ago
|
||
Ah, actually it's ncpus + 4: static size_t ThreadCountForCPUCount(size_t cpuCount) { // Create additional threads on top of the number of cores available, to // provide some excess capacity in case threads pause each other. static const uint32_t EXCESS_THREADS = 4; return cpuCount + EXCESS_THREADS; } I checked that the Talos machines report having a thread count of 12. So, the extra memory is fully accounted for.
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5de9bf11b9
Assignee: nobody → benjamin
Comment 8•9 years ago
|
||
Thank you for the back out, and sorry again for the bad suggestion.
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #8) > Thank you for the back out, and sorry again for the bad suggestion. No problem. I realize sometimes you have to do something to realize it's a bad idea.
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cd5de9bf11b9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 11•9 years ago
|
||
yay, this looks to be resolved by viewing the graph!
You need to log in
before you can comment on or make changes to this bug.
Description
•