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

refactor: Synchronize naming of GC public API #3652

Merged
merged 4 commits into from Jan 5, 2024

Conversation

WojciechMazur
Copy link
Contributor

@WojciechMazur WojciechMazur commented Jan 5, 2024

  • Rename public scalanative_* functions to scalanative_GC_* to keep Boehm GC naming convention (eg. GC_alloc, GC_collect)
  • Synchronize remaining GC env variables to use GC_ prefix instead of SCALANATIVE_
  • Merge spurious GCScalaNative.h into existing ScalaNativeGC.h header defining a common API GC should implement or stub.

@ekrich
Copy link
Member

ekrich commented Jan 5, 2024

Great that you are finishing this up. Just as a note, there could be a couple more that can be shared with Boehm - https://github.com/ivmai/bdwgc/blob/master/docs/README.environment.

We also could support shared macros for special GC building if needed too.
https://github.com/ivmai/bdwgc/blob/master/docs/README.macros

@WojciechMazur
Copy link
Contributor Author

Thank you for the links Eric! Yes, some of these might be integrated as well, but this might require some additional work. One interesting would be adding improved logging mechanism, altough the BoehmGC specificaiton seems to not be perfect. It is using:

  • GC_PRINT_STATS - Turn on GC logging.
  • GC_LOG_FILE - The name of the log file. Stderr by default.
  • GC_PRINT_VERBOSE_STATS - Turn on even more logging.

I don't think we should follow that. Instead we should define GC_LOG_ENABLE which would define GC_PRINT_STATS for Boehm GC under the hood, and do similary with GC_LOG_VERBOSE which should set a BoehmGCs GC_PRINT_VERBOSE_STATS.

In the Immix/Commix to should redirect all outputs within DEBUG_PRINT macros to the log file if it's enabled. We would need to create some dedicated function for that.
We also probably should keep a seperate ENABLE_GC_STATS, GC_STATS_FILE only for the stats (we might adjust the naming here as well). These might produce CSV output and can be produced concurrently by multiple threads to different output files (in Commix).

Because it would require way more work I'd leave it for a follow up PR at some point in the future or if some volunteer shows up.

@ekrich
Copy link
Member

ekrich commented Jan 5, 2024

Sure thing. I agree that the Boehm macros are not ideal.

I don't think we should follow that. Instead we should define GC_LOG_ENABLE which would define GC_PRINT_STATS for Boehm GC under the hood, and do similary with GC_LOG_VERBOSE which should set a BoehmGCs GC_PRINT_VERBOSE_STATS.

This is certainly a good idea.

@WojciechMazur WojciechMazur merged commit 8655125 into scala-native:main Jan 5, 2024
62 checks passed
@WojciechMazur WojciechMazur deleted the refactor/gc-names branch January 5, 2024 18:19
@@ -262,6 +262,9 @@ are used on all GCs. The last one works on Boehm and Commix.
* GC_INITIAL_HEAP_SIZE (was SCALANATIVE_MIN_HEAP_SIZE)
* GC_MAXIMUM_HEAP_SIZE (was SCALANATIVE_MAX_HEAP_SIZE)
* GC_NPROCS (was SCALANATIVE_GC_THREADS)
* GC_TIME_RATIO (was SCALANATIVE_TIME_RATIO)
* GC_FREE_RATION (was SCALANATIVE_FREE_RATIO)
Copy link
Member

Choose a reason for hiding this comment

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

Oops, this should have been GC_FREE_RATIO. Seems to be fine in the code.

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

2 participants