Add access to GC runtime profile stats by marcioapm · Pull Request #2155 · dlang/druntime

marcioapm · 2018-03-28T10:55:45Z

Would be very helpful to get this in the next public release.

wilzbach · 2018-03-28T11:03:44Z

This will need a changelog entry - see the changelog folder.

dlang-bot · 2018-03-28T11:35:34Z

Thanks for your pull request and interest in making D better, @marcioapm! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2155"

wilzbach · 2018-03-28T22:11:58Z

@marcioapm FYI as this is a new symbol it requires approval from @andralex.
I'm also CCing @rainers and @DmitryOlshansky who are our GC experts.

andralex · 2018-03-31T23:39:31Z

src/core/memory.d


    extern (C) BlkInfo_ gc_query( void* p ) pure nothrow;
    extern (C) GC.Stats gc_stats ( ) nothrow @nogc;
+    extern (C) GC.ProfileStats gc_profileStats ( ) nothrow @nogc;


@safe too please

andralex · 2018-03-31T23:39:49Z

src/core/memory.d

+        /// total time spent doing GC in hnsecs
+        long totalCollectionTime;
+        // largest time spent doing one GC cycle in hnsecs
+        long maxCollectionTime;


andralex · 2018-03-31T23:39:54Z

src/core/memory.d

+        /// total number of GC cycles
+        size_t numCollections;
+        /// total time spent doing GC in hnsecs
+        long totalCollectionTime;


andralex · 2018-03-31T23:40:16Z

src/core/memory.d

+     * See `core.memory.GC.ProfileStats` for list of available metrics.
+     * Requires GC profiling to be on
+     */
+    static ProfileStats profileStats() nothrow


marcioapm · 2018-04-02T10:23:05Z

Will this be included in the next public release?

wilzbach · 2018-04-02T10:32:00Z

@marcioapm We can't promise this as the next master-stable branch-off is in two weeks, but as Andrei has already approved this, it looks pretty good.
This just needs to be looked at by one of our GC experts - I don't see anything else that's blocking this.

rainers · 2018-04-02T10:47:27Z

I'm not a huge fan of adding interface members for every stat function which can be specific to a single GC. I think there should be a more generic way to request information from the GC, e.g. using a tagged union. Please also note that there are other PRs adding similar functions: #2052, #1846.

Maybe we should also switch GC from an interface to a base class so that it can have (empty) default implementation. That way it might be easier to add functions without having to update every GC. In addition it could result in a tiny performance improvement because it doesn't need the interface thunks that adjusts the class instance offset.

mihails-strasuns-sociomantic · 2018-04-03T13:18:52Z

I would expect this to be nested under regular GC.Stats

marcioapm · 2018-04-03T15:00:03Z

Filling in GC.Stats is not O(1), requires a lock, and doesn't depend on GC profiling being on.
They are somewhat related, but GC.Stats seems to be about memory statistics, at least in it's current form, whilst GC.ProfileStats is about performance.

mihails-strasuns-sociomantic · 2018-04-03T15:48:01Z

You don't expect to call ProfileStats in a hot loop, do you? Because implementation of actual GC profiling is very far from being usable for that (I tweak that a bit in #1846 but that is still different league of performance).

The fact that it doesn't depend on GC profiling being on is no different for actual GC profiling data - it is always compiled as part of runtime and available to be used. So if you link it with some modules that are compiled with -profile=gc, it will get populated, and otherwise it will stay zero. But it is still there all the time - and in presence of shared libraries it is not even known if it will stay zero if nothing uses GC profiling right now. This is exactly why I expect it to be an inherent part of GC stats.

mihails-strasuns-sociomantic · 2018-04-03T15:51:44Z

Oh, stupid me. I have read PR title and assumed it is also about -profile=gc stats - and only now paid attention to actual content - which seems to be for --DRT-opt profile thing.

Nevermind.

marcioapm · 2018-04-04T08:30:57Z

This is failing because of a shell script.

[libdparse] Running shell script
+ cd test
+ ./run_tests.sh
find: ‘../stdx-allocator/source’: No such file or directory
Compiling unit tests...../src/dparse/parser.d(9): Error: module `mallocator` is in file 'stdx/allocator/mallocator.d' which cannot be read
import path[0] = ../src/
import path[1] = ../stdx-allocator/source
import path[2] = /var/lib/jenkins/dlang_projects@3/distribution/bin/../imports
+ find test/coverage -type f -exec mv {} . ;
find: ‘test/coverage’: No such file or directory
script returned exit code 1```

wilzbach · 2018-04-04T08:35:42Z

Don't worry about Jenkins - it's currently broken and known -> dlang/ci#190 (and here's yours: dlang/ci#193)

rainers

I'm not very strongly against adding this. We might want to record timings unconditionally, though, to avoid the complication of having to enable it. These are only a couple of operations per collection, so it should not have a noticable effect.

On the other hand, you can also access these GC specific variables with a hack, e.g.:

import core.time;
import core.demangle;
extern __gshared pragma(mangle, mangle!Duration("gc.impl.conservative.gc.maxPauseTime")) Duration maxPauseTime;

rainers · 2018-04-15T14:35:24Z

src/core/memory.d

+        /// total number of GC cycles
+        size_t numCollections;
+        /// total time spent doing GC in hnsecs
+        ulong totalCollectionTime;


I think it would be better to keep Duration as the time type.

rainers · 2018-04-15T14:37:34Z

src/gc/impl/conservative/gc.d

+
+        ret.numCollections = numCollections;
+        ret.totalCollectionTime = (prepTime + markTime + sweepTime + recoverTime).total!"hnsecs";
+        ret.maxCollectionTime = maxPauseTime.total!"hnsecs";


collection time and pause time are not the same. The latter is the time that all other threads are suspended aka the world is stopped.

MartinNowak · 2018-04-17T08:08:37Z

Please only use milestones for things that MUST be in the next release, e.g. because it completes an already merged PR. Please do not use them for changes you want in the next release or for general planning. We're releasing often enough so that nobody has to run for the release train.
Milestones are used as a communication tool so we don't miss to release essential PRs.

I understand that GH's issue tracker and milestone feature is designed for and often used for planning in other contexts though.

jacob-carlborg · 2018-05-22T06:56:00Z

What is the status of this?

marcioapm · 2018-09-11T09:51:18Z

@rainers I think I got all your concerns addressed.

rainers · 2018-09-12T05:57:40Z

LGTM but the Requires GC profiling to be active. hints are no longer valid.

The CI failures seem unrelated. core.sys.posix.stdio is not compiled into the druntime lib for some reason...

rainers · 2018-09-12T06:14:43Z

src/core/memory.d

+        Duration totalCollectionTime;
+        /// total time threads were paused doing GC
+        Duration totalPauseTime;
+        // largest time threads were paused during one GC cycle


ddoc /// here and below, too.

jacob-carlborg · 2018-09-12T06:22:41Z

src/core/memory.d


+    /**
+      * Aggregation of current profile information, requires profiling to be on
+     */


The * are not aligned. I recommend using /// instead, which also will save two lines.

rainers · 2018-09-13T06:52:42Z

src/core/memory.d

+    /**
+     * Returns runtime profile stats for currently active GC implementation
+     * See `core.memory.GC.ProfileStats` for list of available metrics.
+     * Requires GC profiling to be on


Still mentions GC profiling needs to be on.

rainers · 2018-09-13T06:53:02Z

src/gc/gcinterface.d


+    /**
+     * Retrieve profile statistics about garbage collection.
+     * Useful for debugging and tuning. Requires profiling to be on.


rainers · 2018-09-23T16:54:33Z

Closing and reopening to retrigger CIs.

PetarKirov · 2018-10-18T07:28:51Z

Rebased in #2332.

PetarKirov · 2018-10-18T07:32:51Z

I didn't intend to close this PR, GitHub somehow automatically did. I rebased @marcioapm's branch locally, but I couldn't force push it to his branch as the PR got closed, so I opened #2332 - let's continue the discussion there.

marcioapm requested review from DmitryOlshansky, MartinNowak, andralex, rainers and wilzbach as code owners March 28, 2018 10:55

wilzbach assigned DmitryOlshansky and rainers Mar 28, 2018

wilzbach added the @andralex Approval from Andrei is required label Mar 28, 2018

andralex approved these changes Mar 31, 2018

View reviewed changes

wilzbach removed the @andralex Approval from Andrei is required label Apr 2, 2018

wilzbach added this to the 2.080.0 milestone Apr 2, 2018

rainers reviewed Apr 15, 2018

View reviewed changes

MartinNowak removed this from the 2.080.0 milestone Apr 17, 2018

mihails-strasuns-sociomantic mentioned this pull request May 25, 2018

Modify GC.stats to allow requesting specific stats #2193

Open

marcioapm force-pushed the master branch from 3f227ec to 2d9eeed Compare September 11, 2018 09:19

rainers reviewed Sep 12, 2018

View reviewed changes

jacob-carlborg reviewed Sep 12, 2018

View reviewed changes

rainers reviewed Sep 13, 2018

View reviewed changes

rainers closed this Sep 23, 2018

rainers reopened this Sep 23, 2018

PetarKirov closed this Oct 18, 2018

PetarKirov force-pushed the master branch from 10286b1 to 7b4172e Compare October 18, 2018 07:20

PetarKirov mentioned this pull request Oct 18, 2018

Add access to GC runtime profile stats #2332

Merged

Uh oh!

Comments

Conversation

marcioapm commented Mar 28, 2018

Uh oh!

wilzbach commented Mar 28, 2018

Uh oh!

dlang-bot commented Mar 28, 2018

Bugzilla references

Testing this PR locally

Uh oh!

wilzbach commented Mar 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcioapm commented Apr 2, 2018

Uh oh!

wilzbach commented Apr 2, 2018

Uh oh!

rainers commented Apr 2, 2018

Uh oh!

mihails-strasuns-sociomantic commented Apr 3, 2018

Uh oh!

marcioapm commented Apr 3, 2018

Uh oh!

mihails-strasuns-sociomantic commented Apr 3, 2018

Uh oh!

mihails-strasuns-sociomantic commented Apr 3, 2018

Uh oh!

marcioapm commented Apr 4, 2018

Uh oh!

wilzbach commented Apr 4, 2018

Uh oh!

rainers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinNowak commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacob-carlborg commented May 22, 2018

Uh oh!

marcioapm commented Sep 11, 2018

Uh oh!

rainers commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainers commented Sep 23, 2018

Uh oh!

PetarKirov commented Oct 18, 2018

Uh oh!

PetarKirov commented Oct 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

MartinNowak commented Apr 17, 2018 •

edited

Loading