381199 - revisit cycle collector aging strategy

Status: RESOLVED FIXED

Description

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

Once we've switched the cycle collector to using the tracing API, I want to revisit the performance/footprint tradeoff in the cycle collector's aging strategy (mScanDelay).  I suspect that, given the current frequency of cycle collections, we're sacrificing too much footprint for negligible performance benefit.  In fact, it wouldn't surprise me if there were no performance benefit to aging at all (and in fact, it's possible there's a performance loss, since it would increase JS GC times).

(Also consider the interaction with the patch to suspect all native wrappers from bug 368869.)

Comment 1

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

Created attachment 268108 [details] [diff] [review]
change mScanDelay default to 0

I haven't had the chance to do measurement, but my gut feeling is that this is what we want given the current frequency of cycle collections, so I'd like to do this; maybe I'll have a chance to measure later.

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 268108 [details] [diff] [review]
change mScanDelay default to 0

Ok, let's try it.

Comment 3

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

Checked in to trunk.

Comment 4

[Michael Ventnor]

This may have caused a huge regression in leaks:

Rlk up to 816KB
Lk up to 3.71MB

Comment 5

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

Backed out due to Lk, RLk, and MH increase.

Maybe bug 368869 would help?  Or maybe the collector was Fault-ing?

Comment 6

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

I relanded this after relanding bug 368869, and the leak regression was still present.

Comment 7

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

Relanded again (and it stuck this time).

Comment 8

[Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.)]

Is there still something to do here or could this be closed?

Comment 9

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

Might be nice to actually measure performance and memory use characteristics.  Probably the first thing to measure would be 0 vs. 1 for performance -- if there's no difference, it's probably not worth bothering to measure further.

Comment 10

[Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.)]

How would one measure the performance here. Running tp/tdhtml or 
using COLLECT_TIME_DEBUG to get some data and trying to interpret it
in some reasonable way, or what?
Btw, bug 373462/bug 385322 will change ccollecting scheduling. Not
sure if the current patch there is the best possible, but something like
that (or mapping ccollector somehow to mem allocs) is needed.

Comment 11

[Robert Sayre]

(In reply to comment #9)
> Might be nice to actually measure performance and memory use characteristics. 
> Probably the first thing to measure would be 0 vs. 1 for performance -- if
> there's no difference, it's probably not worth bothering to measure further.
> 

Marking this fixed. Please open new bugs for measurement issues.