Expose gc_stats as core.memory.GC.stats #1610
Conversation
|
By removing several fields, it's become rather useless. I put those stats in there because they were useful for debugging and tuning the implementation. |
|
Please discuss / agree on it with @MartinNowak as it was his suggestion. I personally don't care which field set it is as longas current 2 are present. |
|
Original bugzilla issue : https://issues.dlang.org/show_bug.cgi?id=16019 |
Those are not portable to other GCs, not even to the already planned GC changes. We want to get rid of pools and move the freelists to thread local caches. |
| /// total size of GC heap | ||
| size_t heapsize; | ||
| /// free bytes on the GC heap (might only get updated after a collection) | ||
| size_t freesize; |
There was a problem hiding this comment.
Any reason for the alllowercase? Since we are adding something new, we might follow the current style guide, right?
|
Ping |
| @@ -1214,7 +1214,7 @@ class ConservativeGC : GC | |||
| size_t bsize = 0; | |||
|
|
|||
| //debug(PRINTF) printf("getStats()\n"); | |||
| memset(&stats, 0, GCStats.sizeof); | |||
| memset(&stats, 0, core.memory.GC.Stats.sizeof); | |||
There was a problem hiding this comment.
Nitpick: remove the memset because it is redundant for out parameters. (See https://dlang.org/spec/function.html#parameters)
There was a problem hiding this comment.
It is a bad idea to put unrelated changes in pull requests. Please open a new one.
|
Some improvements in https://github.com/Dicebot/druntime/pull/1. |
Before making stats struct and thus standardizing it, field set it defines has to be reduced to minimal amount meaningful to any GC implementation.
|
Picked changes from @MartinNowak and rebased. Also checked it on simple allocating apps, reported stats match expectations exactly. |
|
Does it make sense to add a test for this in https://github.com/dlang/druntime/tree/master/test? Perhaps in a separate PR? |
Writing a test makes sense, but it shouldn't be too sensitive to different values getting reported. |
|
Auto-merge toggled on |
|
Yeah I am not sure what exactly to test as the stats are supposed to show GC internal memory data, not match exactly allocation volume from the user code. Checking that used stats increase somewhat if GC allocation happens is pretty much only reliable thing. |
|
Oh, almost forgot about it - where should I submit release note update PR? |
If you close the bugzilla issue, it will automatically get included in the changelog. If you want to make a more detailed entry in the changelog, update the |
Picks changes from dlang#1610
Picks changes from dlang#1610
Picks changes from dlang#1610
Replaces #1591 with
changes as suggested by @MartinNowak