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

builtin: add gc_collect/0, gc_get_warn_proc/0, gc_set_warn_proc/1. Use them to turn off GC warnings by default. #20788

Merged

Conversation

spytheman
Copy link
Member

@spytheman spytheman commented Feb 11, 2024

With the new prebuilt libgc.a, I get warnings when running gg programs, similar to this one:

#0 14:26:29 ᛋ master /v/oo❱v run examples/gg/mandelbrot.v
GC Warning: Heap grown by 4600 KiB while GC was disabled

This PR silences them by default, while also giving an API for redirecting them to a user specified callback, if needed.

@spytheman spytheman merged commit 8793aeb into vlang:master Feb 11, 2024
54 checks passed
@spytheman spytheman deleted the builtin_add_gc_collect_and_gc_xxx_warn_proc branch February 11, 2024 14:07
@joe-conigliaro
Copy link
Member

joe-conigliaro commented Feb 11, 2024

Thanks @spytheman, this actually got me thinking...

What do you think about possibly having them left on for debug builds? -c or -cg (even though they are for debug symbols, not debug mode as such)

Or we could add an actual -debug flag which turns on all debug stuff like -cg, and those warnings, and any other things we may want to add in the future which we find useful for debugging.

Unless non -prod builds are technically debug builds, and we only have them off for-prod?

Anyway these just some thoughts which came to my head :)

@JalonSolov
Copy link
Contributor

I like the idea of a -debug flag for this. You don't always want all of that output, but it would be nice to get it all with a single flag.

@spytheman
Copy link
Member Author

I've added -d gc_warn_on_stderr in 101b8a6 .

@joe-conigliaro
Copy link
Member

I've added -d gc_warn_on_stderr in 101b8a6 .

This is good to have.

But the reason I was thinking it would be nice to have a flag that shows all output which could be useful for debugging is because that way it would be much easier for a developer to actually see the warning without specifically turning that on. It's unlikely they would try -d gc_warn_on_stderr without a very specific reason. If they had a flag to show all useful debug info/warnings they might stumble on a bug in their program.

There may be other useful warnings hiding behind -d which could also be shown with such a flag. Just some thoughts.

@spytheman
Copy link
Member Author

that way it would be much easier for a developer to actually see the warning without specifically turning that on

The previous behavior for the old libgc build, did not show those warnings. I've never missed them, and I they are about things, that I can not control, and usually do not care about => I do not want to see them, unless I am specifically debugging a GC issue.

It's unlikely they would try -d gc_warn_on_stderr without a very specific reason.

Imho that is good.

If they had a flag to show all useful debug info/warnings they might stumble on a bug in their program.

Or, they will be overwhelmed by useless info. Consider the output of the current -v flag to V. It is practically unreadable to me. There is no structure in it, there are no descriptions about what is what, and in what order.

@joe-conigliaro
Copy link
Member

joe-conigliaro commented Feb 12, 2024

I was just writing my thoughts which where that it could be handy to see the warning since it's possible in some cases it's caused by an overlooked bug, in which case no one would ever think to try -d gc_warn_on_stderr

There was cases where the old version would still show warnings, perhaps not for that reason.

I agree that a bunch of messages would be overwhelming.

I wasn't pushing for it to be implemented, I was just mentioning what came to my head so I could see what you thought about it.

@spytheman
Copy link
Member Author

spytheman commented Feb 12, 2024

it could be handy to see the warning since it's possible in some cases it's caused by an overlooked bug, in which case no one would ever think to try -d gc_warn_on_stderr

Yes, in such cases, the new behavior of silencing such messages, will make people not aware of the warnings. On one hand, in my experience, the GC warnings were never something I cared about, or that I could do anything to solve them. On the other hand, there were often questions about how to turn them off or avoid them in the Discord server.

There was cases where the old version would still show warnings, perhaps not for that reason.

Yes, usually the reason for GC warnings, is programs, that allocate big chunks of memory in a loop (over 32MB). Sometimes that is a signal for a bug in the program. Sometimes it is what the program has to do. The GC does not have enough information to decide which is which, it just has a heuristic, that triggers. In the second case however, the warnings were hard to turn off.

On my Linux machine, even just this, produces a warning:

#0 10:17:40 ᛋ patch-1 /v/oo❱v -d gc_warn_on_stderr run examples/gg/minimal.v
GC Warning: Heap grown by 128 KiB while GC was disabled

I have no idea why, and I doubt, that it is indicative of a bug, that we can solve.

I wasn't pushing for it to be implemented, I was just mentioning what came to my head so I could see what you thought about it.

I think that automatic systems (like the GC), compiled in every V program, should be preferably silent, leaving all output to be what the user programmed explicitly.
I also think, that the warnings by the GC, are not useful to the vast majority of users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants