Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up mock_constructor by removing gc.collect #335

Closed
wants to merge 1 commit into from

Conversation

deathowl
Copy link
Member

Summary:
mock_aiog
0:00:00.064367
from this calling gc.collect took:
0:00:00.053297 and it's synchronous

since default slow callback detection is 0.1 if someone uses multiple mock_constructor calls these gc.collect-s add up and exceed the threshold

Differential Revision: D38073077

Summary:
mock_aiog
0:00:00.064367
from this calling gc.collect took:
0:00:00.053297 and it's _synchronous_

since default slow callback detection is 0.1 if someone uses multiple mock_constructor calls these gc.collect-s add up and exceed the threshold

Differential Revision: D38073077

fbshipit-source-id: e43aa128224b0ac14403c26057755c2268a0748d
@facebook-github-bot facebook-github-bot added CLA Signed Do not delete this pull request or issue due to inactivity. fb-exported labels Jul 22, 2022
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D38073077

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2718898253

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.002%) to 94.116%

Totals Coverage Status
Change from base Build 2710955248: -0.002%
Covered Lines: 2639
Relevant Lines: 2804

💛 - Coveralls

@fornellas
Copy link
Contributor

IIRC this GC had to be there for consistency at the check. I belive removing it, will lead to inconsistent corner cases where it'll complain mock will fail, wrongly.

@deathowl
Copy link
Member Author

It did not trigger any test faillures on the internal tests,but actually the false slow calllback detection becaause of GC blocking the event loop caused more faulty signals, so the change is reasonable.

@fornellas
Copy link
Contributor

I don't think i'd trigger any consistent build issues. IIRC, without this GC, depending on how objects are handled up to that point, there MAY be lingering references the GC would clean up, that, without cleanup, it would then trigger false errors. So, it'd be the kind of unexpected inconsistent behaviour, rather than always failing.

I recommend considering alternatives, such as wrapping this GC run with a temporary disable of the event loop block check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. fb-exported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants