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)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jmaher, Assigned: Benjamin)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

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)
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)
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)
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)
> 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 :(
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.
Thank you for the back out, and sorry again for the bad suggestion.
(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.
https://hg.mozilla.org/mozilla-central/rev/cd5de9bf11b9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.