Fix #3237: Configure default GC settings at compile time #4338

ekrich · 2025-05-16T20:23:42Z

The only thing really to note is that for Boehm you need to define with memory in bytes. I couldn't really verify that the setting worked as it doesn't seem to really allocate the initial heap right away but looking at the code it seems to be ok. Plus, if I try to set 3m or something, the build fails because the #define passed on the command line needs to be an int. Our memory parser also just passes back the bytes if passed to it so you can set the other GCs with bytes.

Docs

Note: in the clang-format it seems if you have a defined function with an empty body either fun{}; or fun {} are ok. you don't really need a semi-colon on the first one so the second one makes more sense.

ekrich · 2025-05-22T16:39:07Z

Some test results for gc none. Note that compiled in settings can be overridden by env vars for a nice extra. Note: the default is 64m.

% ./sandbox/.2.13/target/scala-2.13/sandbox
GC_THREAD_HEAP_BLOCK_SIZE=32m
Hello, World!
% export GC_THREAD_HEAP_BLOCK_SIZE=128m                                     
% ./sandbox/.2.13/target/scala-2.13/sandbox
GC_THREAD_HEAP_BLOCK_SIZE=128m
Hello, World!

ekrich · 2025-05-22T16:45:49Z

Test for immix:

% ./sandbox/.2.13/target/scala-2.13/sandbox
GC_INITIAL_HEAP_SIZE=5m
GC_MAXIMUM_HEAP_SIZE=5m
Hello, World!
% export GC_INITIAL_HEAP_SIZE=10m             
% export GC_MAXIMUM_HEAP_SIZE=20m
% ./sandbox/.2.13/target/scala-2.13/sandbox
GC_INITIAL_HEAP_SIZE=10m
GC_MAXIMUM_HEAP_SIZE=20m
Hello, World!

ekrich · 2025-05-22T17:33:35Z

Memory values can be debugged via -DDEBUG_PRINT added to build.

ekrich · 2025-05-22T18:46:38Z

Looks like scalafmt had a core dump.

nativelib/src/main/resources/scala-native/gc/shared/Parsing.c

Co-authored-by: Wojciech Mazur <wojciech.mazur95@gmail.com>

ekrich · 2025-05-24T02:04:11Z

Ok, I have no idea what is wrong with the formatting.

ekrich · 2025-05-24T02:22:15Z

Ok, confirmed it still work with env override.

LeeTibbert · 2025-05-24T11:50:08Z

Set out to complete the requested review.

Because of discussions in this topic, I had expected the build failure to be about code formatting,
possibly C code. Turns out the failure is in macOS scripted#shutdown. I do not understand the
failure and suspect that it is an intermittent or GitHub environment failure.

LeeTibbert · 2025-05-24T11:57:25Z

Memory values can be debugged via -DDEBUG_PRINT added to build.

Adding visibility of the internal values is a good step. I am a big fan
of namespaces. Could you consider putting this in a namespace?

Now, which namespace is a somewhat harder issue; the dead hand of
prior art.

It would be nice if it were somewhat like "SCALANATIVE_GC_DEBUG_PRINT".
That assumes that there is only one GC active at any given time.

A lesser, IMO, possibility would be "GC_DEBUG_PRINT". I do not
like use of "GC_foo" in many places, but I suspect that it is what the
gcs use (inherited from Boehm?).

docs/user/runtime.md

javalib/src/main/resources/scala-native/ifaddrs.c

nativelib/src/main/resources/scala-native/gc/shared/Parsing.c

ekrich · 2025-05-24T13:27:49Z

About the DEBUG_PRINT suggestion, I used the existing one being used. The idea to align with Boehm with the shared GC_ saves us time and make it much easier for users. Boehm still is very useful in some situations and has good support. Also, the history of this being our first GC is hard to ignore. I plan to come back to GC again so maybe we can make more progress in the unification of "defines". There isn't much chance of conflict here versus the flat namespace of bindings from everywhere.

I already got myself into more because of this - ivmai/bdwgc#735

LeeTibbert · 2025-05-24T13:48:53Z

Still working on review...

About the DEBUG_PRINT suggestion, I used the existing one being used.

I understand & appreciate not wanting to break the world. SN is not 1.0 yet,
so we have some flexability, I believe.

We both have many demands on our time, especially on a long holiday weekend, so I
do not want to make this too important.

Perhaps you/we could keep the existing DEBUG_PRINTF in the code but
mark it as deprecated and subject to removal before SN 1.0 and add a new
"GC_DEBUG_PRINTF" or such? Only the new & consistent name would
be documented.

Do you know who added and when the existing idiom was added?

nativelib/src/main/resources/scala-native/gc/shared/Parsing.c

LeeTibbert

Looks Good To Me - LGTM. Thank you.

LeeTibbert · 2025-05-24T14:37:49Z

A discussion of C style, getting this puppy shipped as-is is much more important in my mind.

Different authors will do things different ways. One of the things I like about Scala Native
development is that devos are given wide latitude, as long as the code is correct.

There is a block is correct but I think might be simplified. Of course, one person's simplification
is another's anathema.

#if defined(GC_INITIAL_HEAP_SIZE)
    if (strcmp(envName, "GC_INITIAL_HEAP_SIZE") == 0) {
        return VAL_STR(GC_INITIAL_HEAP_SIZE);
    }
#endif

#if defined(GC_MAXIMUM_HEAP_SIZE)
    if (strcmp(envName, "GC_MAXIMUM_HEAP_SIZE") == 0) {
        return VAL_STR(GC_MAXIMUM_HEAP_SIZE);
    }
#endif

#if defined(GC_THREAD_HEAP_BLOCK_SIZE)
    if (strcmp(envName, "GC_THREAD_HEAP_BLOCK_SIZE") == 0) {
        return VAL_STR(GC_THREAD_HEAP_BLOCK_SIZE);
    }
#endif

Perhaps I've been reading & writing too much Scala recently, but the "if defined()"s somehow
disquiet me.

A voice is telling me that GC_* should always be defined. That is, at the top of the file
(almost certainly not in the corresponding .h file, since other files are not expected to
use the values) one would have a section roughly (not compiled)

#if !defined(GC_Foo_1)
#  define GC_Foo_1   sensible_default
#endif

Then one could remove the '#if defined()' lines (and corresponding #endif) and
the could would be easier (for me) to follow.

Eric, you know the problem domain. Is there a better sensible for each of the three, across GCs?
With the given STR_VAL I think the following would work if there is none such:

#include <stdlib.h>
#if !defined(GC_Foo_1) // Use command line definitions if present
#  define GC_Foo_1   NULL
#endif

Will give you the NULL return that happens now. That keeps a lot of the C macro
stuff confined to one place.

When I am on my long walk, I'll let my remaining disquiet of the 3 strcmp()s percolate.
There are only 3 of them, not 300, so it is below the weeds in importance. They just
evoke the voice of my Algorithms professor. I really do have to Get A Life.

Thanks for listening/reading.

ekrich · 2025-05-27T15:16:12Z

Lee, on the last comment we need to check which ENV var is being asked for at runtime. It just returns the defined value if there it one. I hope that makes sense because if you follow the call chain, at runtime the parsing of the memory value still occurs even if it is compiled in.

Edit: I only answered part of your question. The user can decide to only set the MIN or the MAX memory or both and the default case is none are defined so the whole #if defined section is blank. The env vars passed in are from the lookup code - they should always be there but there are NULL checks in case. To me an assert or static_assert is possible is a better than a NULL check.

ekrich · 2025-05-27T15:55:42Z

@WojciechMazur I think this is ready to go now.

ekrich · 2025-05-27T16:08:38Z

@LeeTibbert I added a ticket about the debug - #4350

ekrich requested review from WojciechMazur and LeeTibbert May 22, 2025 18:44

WojciechMazur reviewed May 23, 2025

View reviewed changes

nativelib/src/main/resources/scala-native/gc/shared/Parsing.c Outdated Show resolved Hide resolved

ekrich and others added 8 commits May 23, 2025 18:18

Fix scala-native#3237: Configure default GC settings at compile time

9a64475

Try and fix clang-format

df3faae

Try and fix clang-format 2

19cc2a8

Try and fix clang-format 3

756b89f

Update parsing with debug and shared code path

82006f4

Update docs

53491f6

Update Parsing method

0c5f4eb

Co-authored-by: Wojciech Mazur <wojciech.mazur95@gmail.com>

Add and adjust comments and formatting

5d401c4

ekrich force-pushed the fix/gc-vals branch from 4970593 to 5d401c4 Compare May 24, 2025 01:29

Move comment for clang format

3fd3aaa

Fixed formatting?

2aee145