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

fix(gnolang): don't print debug logs for package unicode #1226

Closed
wants to merge 1 commit into from

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Oct 11, 2023

Anyone who's worked with DEBUG=1 probably knows the pain of watching the screen scroll through for 5 minutes while you wait for the parsing of package unicode to have finished.

This PR aims to fix that. This is a scratch-an-itch PR, and I want to tackle debugging systems better in the future, but this should already improve massively the usefulness of DEBUG=1.

@thehowl thehowl requested review from jaekwon, moul and a team as code owners October 11, 2023 01:30
@thehowl thehowl changed the title fix(gnolang): dont print debug logs for package unicode fix(gnolang): don't print debug logs for package unicode Oct 11, 2023
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Oct 11, 2023
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (e144d26) 47.80% compared to head (3d06eba) 47.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1226      +/-   ##
==========================================
- Coverage   47.80%   47.75%   -0.05%     
==========================================
  Files         369      369              
  Lines       62710    62717       +7     
==========================================
- Hits        29981    29953      -28     
- Misses      30308    30334      +26     
- Partials     2421     2430       +9     
Files Coverage Δ
gnovm/tests/file.go 49.38% <ø> (-0.25%) ⬇️
gnovm/pkg/gnolang/debug.go 4.08% <0.00%> (-0.37%) ⬇️
gnovm/pkg/gnolang/machine.go 39.45% <0.00%> (-0.15%) ⬇️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moul moul requested a review from piux2 October 11, 2023 10:45
@moul
Copy link
Member

moul commented Oct 12, 2023

Another option is to use the AST caching feature to preload standard libraries without DEBUG=1. After that, you can enable DEBUG=1 again for userland packages that are not cached.

Additionally, as we discussed, it is worth considering the native injection of large libraries on a per-case basis.

If we proceed with the current pull request, we can improve the code's readability by introducing a variable called var skipDebugPackages map.

Copy link
Contributor

@piux2 piux2 left a comment

Choose a reason for hiding this comment

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

What about the people who do want to catch the unicode debugging information? Are there any alternative solutions?

@thehowl
Copy link
Member Author

thehowl commented Oct 19, 2023

What about the people who do want to catch the unicode debugging information? Are there any alternative solutions?

I'm gonna give them 10 bucks if they can make sense out of the huge pile of debugging information that is printed during unicode preprocessing, without doing any code modifications.

Seriously, I think we obviously need to have more granular debugging, allowing the user to select exactly what information they need. A larger refactoring of debugging is needed, and is one of the topics of the company retreat, and one that I want to tackle myself personally (see the first item in https://github.com/gnolang/hackerspace/pull/44/files ).

If you think that somebody might have a hard time figuring out what's going on, I can add a debug message before entering package unicode saying "unicode logs are disabled as they are too verbose; to enable them change $file_name"

@moul moul mentioned this pull request Oct 20, 2023
@piux2
Copy link
Contributor

piux2 commented Oct 23, 2023

What about the people who do want to catch the unicode debugging information? Are there any alternative solutions?

I'm gonna give them 10 bucks if they can make sense out of the huge pile of debugging information that is printed during unicode preprocessing, without doing any code modifications.

Seriously, I think we obviously need to have more granular debugging, allowing the user to select exactly what information they need. A larger refactoring of debugging is needed, and is one of the topics of the company retreat, and one that I want to tackle myself personally (see the first item in https://github.com/gnolang/hackerspace/pull/44/files ).

If you think that somebody might have a hard time figuring out what's going on, I can add a debug message before entering package unicode saying "unicode logs are disabled as they are too verbose; to enable them change $file_name"

I totally agree that we need to address the debugging issue systematically; currently, 'dlv' is the only effective tool for debugging VM issues.

The issue is that the PR completely disabled the unicode debug information without providing an alternative. Additionally, people don’t necessarily read these debug information on screen; they can dump it into a file and search through it manually, or run scripts to parse the information they are looking for.

We pay extra attention to pkg/gnolang and tm2 as they are not merely libraries used by gno.land. We aim to treat the source code in these sections as standalone products in themselves

How about we create a separate debug level for things we don't want to see at the first place?

@thehowl thehowl mentioned this pull request Nov 2, 2023
@thehowl
Copy link
Member Author

thehowl commented Nov 7, 2023

I opened this as a quickfix, awaiting a proper improved debugging system. Of course, an improved version of debugging can have different log levels and selectively discard showing debug logs for imported packages. However, that is much out of the scope of this PR.

I'll leave the actual fix for this to be done by #999.

@thehowl thehowl closed this Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: 🔵 Not Needed for Launch
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants