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

GC/Boehm: Silence GC warnings about big allocations. #11289

Merged
merged 4 commits into from
Oct 24, 2021

Conversation

yxhuvud
Copy link
Contributor

@yxhuvud yxhuvud commented Oct 7, 2021

These warnings can happen for example when repeatedly adding elements
to big arrays or hashes. Unfortunately these warnings are not very
helpful and usually just irritating.

Fixes #11274

These warnings can happen for example when repeatedly adding elements
to big arrays or hashes. Unfortunately these warnings are not very
helpful and usually just irritating.
@straight-shoota
Copy link
Member

Can you please add a comment that explains the reasoning directly in the code?

src/gc/boehm.cr Outdated
@@ -126,6 +126,7 @@ module GC
{% end %}
LibGC.init

LibGC.set_warn_proc ->(_msg, _v) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment explaining why we are doing this?

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @yxhuvud 🙏

@straight-shoota straight-shoota added this to the 1.3.0 milestone Oct 7, 2021
@luislavena
Copy link
Contributor

luislavena commented Oct 17, 2021

Hello folks,

I would like to add two issues that were connected to this:

Merging this one will close those two.

Cheers.

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Oct 18, 2021

Hmm. #11324 seems to be getting warnings like GC Warning: Finalization cycle involving 0x7f6d1fa9c900.

This make me think we should only swallow the repeated allocation warning, and not all other possible warnings. In this case it looks to be an issue about some miswritten finalizer. That seems a lot more helpful than not warning at all.

Thoughts?

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Oct 19, 2021

Thoughts, would you prefer the check for the above to use a regexp to match against /Repeated allocation/ or would you prefer to compare against the full message, ie GC Warning: Repeated allocation of very large block (appr. size %ld):\n\tMay lead to memory leak and poor performance\n". The latter seems more prone to break on minor changes to me. It shouldn't happen often enough to be a performance impacting choice in any case.

@Sija
Copy link
Contributor

Sija commented Oct 19, 2021

Maybe some middle ground (without using regex):

msg.starts_with?("GC Warning: Repeated allocation of very large block")

LibGC.set_warn_proc ->(_msg, _v) do
format_string = String.new(msg)
unless format_string.starts_with?("GC Warning: Repeated allocation of very large block")
LibC.printf format_string, v
Copy link
Contributor Author

@yxhuvud yxhuvud Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opinions? It felt cleaner to use printf than to rewrite the format string, but 🤷‍♂️

@yxhuvud yxhuvud force-pushed the silence_gc_warnings branch from 08b8118 to 8c1e225 Compare October 19, 2021 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GC warnings on large allocations
7 participants