Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Manually run GC on reactor tick. #771

Merged
merged 3 commits into from Jun 7, 2016

Conversation

Projects
None yet
3 participants
Owner

erikjohnston commented May 9, 2016

This also adds a metric for amount of time spent in GC.

Manually run GC on reactor tick.
This also adds a metric for amount of time spent in GC.

@NegativeMjark NegativeMjark and 1 other commented on an outdated diff May 13, 2016

synapse/metrics/__init__.py
@@ -182,6 +184,18 @@ def f(*args, **kwargs):
end = time.time() * 1000
tick_time.inc_by(end - start)
pending_calls_metric.inc_by(num_pending)
+
+ threshold = gc.get_threshold()
+ counts = gc.get_count()
+
+ start = time.time() * 1000
+ for i in [2, 1, 0]:
+ if threshold[i] < counts[i]:
+ logger.info("Collecting gc %d", i)
+ gc.collect(i)
+ end = time.time() * 1000
+ gc_time.inc_by(end - start)
@NegativeMjark

NegativeMjark May 13, 2016

Contributor

Might be interesting to collect stats about the different levels of collection going on.

@erikjohnston

erikjohnston May 13, 2016

Owner

Oooh, yes, good point.

Owner

ara4n commented May 13, 2016

this seems really weird - surely GCs can be very costly with lots of objects, especially for the higher generations? Why would you want to run it on every reactor tick? Wouldn't it be better to bump up the GC thresholds and just have the machine stop the world to GC once every few hours or whatever?

Owner

erikjohnston commented May 13, 2016

@ara4n We check if we need to run the GC on each reactor tick, we don't necessarily do so. The check should be equivalent to the one done internally by python

Owner

ara4n commented May 13, 2016

okay... but what is the rationale for doing so? (aka where are the comments?)

Owner

erikjohnston commented May 13, 2016

Sure I can add a comment. It's mainly so that we can get some metrics over how long we spend GC'ing

Owner

ara4n commented May 13, 2016

aaah! makes much more sense - thanks :)

Contributor

NegativeMjark commented May 16, 2016

LGTM. Might want to be careful about merging this to make sure it doesn't end up on one of our servers by accident.

@erikjohnston erikjohnston merged commit 8c966fb into develop Jun 7, 2016

8 checks passed

Flake8 + Packaging (Commit) Build #681 origin/erikj/gc_tick succeeded in 31 sec
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #662 origin/erikj/gc_tick succeeded in 4 min 43 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #669 origin/erikj/gc_tick succeeded in 5 min 27 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Commit) Build #725 origin/erikj/gc_tick succeeded in 1 min 25 sec
Details
Unit Tests (Merged PR) Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment