search

iu-parfunc / gibbon

A compiler for functional programs on serialized data
http://iu-parfunc.github.io/gibbon/
151 stars 13 forks source link

`--print-gc-stats` not available #241

Closed ulysses4ever closed 5 months ago

ulysses4ever commented 5 months ago

Currently, --print-gc-stats has no effect. I see two reasons for that.

The first reason is a simple bug in codegen discussed in a separate PR #238 (codegen produces 2 unconditional returns and the printing function https://github.com/iu-parfunc/gibbon/blob/64f0447cf928caba10ca9f8ef5f2d493a0be85a9/gibbon-rts/rts-c/gibbon_rts.c#L2032 is only called after the first return, so is never reached).

The second problem was introduced in 09dd49e67540571623f9f0ae32bae96d58c5f1ff, which added a new RTS switch called PRINT_GCSTATS / _GIBBON_PRINT_GCSTATS that guards the call to the printing function. The issue is that the compiler was never updated to use this new switch: it still sets the old switch GCSTATS https://github.com/iu-parfunc/gibbon/blob/64f0447cf928caba10ca9f8ef5f2d493a0be85a9/gibbon-compiler/src/Gibbon/Compiler.hs#L377 when the compiler is called with --print-gc-stats.

I am not sure why the second switch was introduced: it seems to imply that you may sometimes want to collect the GC stats but not print them. This doesn't makes sense to me. My proposal is to remove the new switch and get back to the only switch (GCSTATS) that collects and prints the stats. This will restore the effect of --print-gc-stats.

@ckoparkar as the author 09dd49e67540571623f9f0ae32bae96d58c5f1ff (and the GC itself, of course), do you have an opinion on that? /cc @vollmerm who was interested in GC work in general.

ckoparkar commented 5 months ago

@ulysses4ever I agree that we should only have a single flag for this.

I don't fully recall why I separated out the collection and printing of GC stats. But I think it was related to the unit tests that I was writing in gc_test.rs; I was querying the GC stats there but I did not want to print them. But the printing only happens in the C RTS, so that's not an issue.

ulysses4ever commented 5 months ago

@ckoparkar thank you for clarifying! I had a hunch that the distinction may be useful for testing... But it's very good point that since printing is done in C and testing is in Rust, one switch may work just fine. I implement it in #243. Would appreciate an approval!