Expose a GC.statistics() function.

[alexrp]

Second take on #233. Also includes a fullCollections property.

[alexrp]

Any input on this? The code itself is trivial, but what I really want to know is whether the amount of technical debt (and there's a lot of that here) this introduces is acceptable.

[complexmath]

I hadn't done anything about GC statistics yet because I wanted to have a discussion about exactly what metrics would be useful, and also consider how easy it would be to provide those across GC implementations. For impact, info gathering could be enabled/disabled via a flag. I mostly want to make sure that the bulk of GC implementations could provide the info whatever stats functionality we add exposes.

[alexrp]

I think that for the info that a GC implementation cannot provide, simply returning 0 or perhaps -1 is reasonable (documenting the lack of some stats feature(s) would also be a good idea). The thing about the statistics API is that it's only there for performance analysis and tuning, so strong guarantees about availability of information don't have to be made, IMO. Your thoughts on this?

Also, how do you envision enabling/disabling gathering of statistics should work?

[andralex]

I've put @complexmath on this. Regarding enabling/disabling statistics collection, I think we should have a property there. The big question is whether statistics could and should be enabled per thread or globally.

[alexrp]

Per-thread would be significantly more involved. I'm not opposed to it, but I think we can add per-thread statistics after we add support for global statistics gathering.

@andralex @complexmath any input on my point about statistics that a GC cannot provide (i.e. just return 0)? Does it seem reasonable enough?

[andralex]

A few more thoughts after making one more pass through this:

1. There should be stats per thread and an ability to consolidate them all per process.
2. Stats collection should only be in a special build, see http://linux.die.net/man/3/jemalloc
3. The actual stats collected should e.g. mimic jemalloc's stats, see again http://linux.die.net/man/3/jemalloc

[andralex]

Oh, and the stats structure should have default initializers that don't make sense, e.g. fullCollections is currently 0 by default but it could actually be realistically zero. It should be size_t.max.

[alexrp]

There should be stats per thread and an ability to consolidate them all per process.

Agree.

Stats collection should only be in a special build, see http://linux.die.net/man/3/jemalloc

This sounds extremely complicated. Our build situation is already quite the maintenance nightmare...

The actual stats collected should e.g. mimic jemalloc's stats, see again http://linux.die.net/man/3/jemalloc

I think it only makes sense to collect stats that are actually relevant to the GC implementation, however. There seems to be a lot of excess information in jemalloc that I am far from convinced anyone would ever need in a garbage-collected scenario.

Oh, and the stats structure should have default initializers that don't make sense, e.g. fullCollections is currently 0 by default but it could actually be realistically zero. It should be size_t.max.

I don't follow here. Any GC implementation can provide this particular piece of information, and, logically, they start out with zero collections?

[andralex]

1. My thought was just to define the gc_stats version and make all stats depend on it. No biggie. Leave this to only people who want to build their own druntime have it.
2. It would be useful to see e.g. the total number of bytes allocated, total number of bytes collected etc. These tend to impact performance, which makes the version guard all the more necessary.
3. My only point is that GCstats.init should have only invalid fields that couldn't be reasonably filled with real data. This is to support your unusual practice of always returning a struct, regardless of what stats may or may not be supported or collected.

[alexrp]

My thought was just to define the gc_stats version and make all stats depend on it. No biggie. Leave this to only people who want to build their own druntime have it.

Sorry, you lost me there. Rephrase?

It would be useful to see e.g. the total number of bytes allocated, total number of bytes collected etc. These tend to impact performance, which makes the version guard all the more necessary.

Sure, stuff like that should be there. I was just thinking that exposing every little piece of information in the statistics (which jemalloc seems to do - correct me if I'm wrong) might be a bit overkill.

My only point is that GCstats.init should have only invalid fields that couldn't be reasonably filled with real data. This is to support your unusual practice of always returning a struct, regardless of what stats may or may not be supported or collected.

That was actually just an artifact of code already in place inside the gc package. But okay, if GCStats.init contains invalid values, then GCs that support some particular values can just fill in as needed. Makes sense.

[andralex]

Rephrasing "My thought was just to define the gc_stats version and make all stats depend on it. No biggie. Leave this to only people who want to build their own druntime have it."

->

"Let's define version=gc_stats; and then every action related to collecting statistics would go inside version(gc_stats) { ... } blocks."

I agree that it's a bit onerous to require a versioned build to collect GC statistics. My opinion goes as follows:

1. There are statistics that don't impact GC performance if collected. Some are mildly interesting.
2. There are statistics that DO impact GC performance if collected. Some are very interesting.

We could go with (1) and not use versioning, but we'd be missing out on (2). Let me ask Jason Evans about how to best approach this.

[andralex]

Oh, there's a 3rd way: use a global Boolean statisticsEnabled that guards all statistics collecting activity. I presume one test should be cheap enough.

[alexrp]

Yes, using a simple Boolean wouldn't really impact performance in any noticeable way and wouldn't complicate the build process. So I take it we'll have a statisticsEnabled property on the GC struct?

[andralex]

Most likely, but let me get @jasone to weigh in on this before greenlighting. Thanks!

[jasone]

I don't have any specific comments on the API under discussion, but here are some general thoughts about allocator statistics. As of jemalloc 3.0.0, all statistics gathering is enabled by default at build time. This adds a small amount of overhead, but without detailed statistics, jemalloc's behavior for diverse applications is very difficult to reason about. GC systems are even more sensitive to application behavior in my experience, so I would recommend gathering every informative statistic that doesn't impose an unacceptable overhead (i.e. constant-time overhead only), and leaving the statistics gathering machinery in place all the time. D's GC will never be "done", and the statistics will aid GC evolution.

[andralex]

Thanks @jasone. I think we're clear with using a run-time enabler for collecting statistics. @alexrp, thanks for this work. Let's take it to fruition in 2.061.

[andralex]

Could we avoid defining this twice?

[alexrp]

Unfortunately, no. core.memory cannot import any part of the gc package due to visibility issues.

[alexrp]

Closing this temporarily to avoid confusion. Will reopen once I do the discussed changes.

[alexrp]

@andralex @jasone I'm getting around to this now, but I need to know if we really want to allow enabling/disabling statistics collection. From looking at the current statistics that are gathered, it seems that adding a branch would be slower than just gathering the statistics unconditionally.

[alexrp]

Also, I just talked to a few folks on IRC, and they all seemed to think that simply returning zero for unsupported statistics is acceptable (and better from an API usability perspective), even if the statistic could realistically be zero.

[alexrp]

@andralex Ping.

[andralex]

I'm against statistics that could be 0 either legally or when uncollected. This is a simple unforced design error, let's just fix it.

[andralex]

We must still have a Boolean guard because it's the more extensible way - we don't know what statistics we'll add in the future, and some of those may actually be costlier than a jump. Also, people may disable and reenable statistics collection for measurement purposes (e.g. ignore the stats for certain portions of the code etc). Thanks.

[alexrp]

@andralex @complexmath New GC statistics interface is done. Please re-review the entire thing.

[andralex]

pure? I'm only asking because I see you marked other functions as such.

[andralex]

Wait, that's reading a global. Rats. I meant const, but that doesn't quite apply :o).

[andralex]

Instead of base+size, how about a void[]?

[alexrp]

Sure.

[andralex]

I'm quite unhappy after reviewing this. It's a clusterfrak of functions forwarding to other functions, and work is actually done only in a fraction of the code written. There's some C-level functions that have awkward names, but then there's some D functions that also have awkward names. Though I fully understand that's not an issue introduced by this diff, I wonder how the hell did we get to this point. Any ideas on how to simplify this?

[complexmath]

I think we're putting the cart before the horse here. The reason I hadn't formally exposed statistics yet was because I didn't know which statistics were most useful and could be generated by most GC implementations. The existing statics were available only via a C call for this reason -- you could get them if you wanted them, but we hadn't made a commitment to the interface or the data returned. I'd like to see some actual discussion / research into what statistics would be most useful and worry about the implementation later.

Andrei, it may be that the ultimate implementation does use a bunch of forwarding functions because this is how the GC interface works. It's primarily intended to avoid the need to import gc.anything. A separate issue might be whether it's reasonable to put an interface declaration in core.memory that a GC can expose. The C interface is really an artifact of earlier days when druntime was three logically separate sub-libraries.

[alexrp]

@andralex I fixed two of the issues you mentioned (size_t.max initialization and white space). I didn't alter the BlockInfo layout because that, for whatever reason, appears to make the GC crash and burn, and I really don't care enough to debug that over a triviality like this right now.

I don't think we can simplify the GC interface situation until we get proper shared libraries. The proxy stuff in particular seems to be a workaround for the lack of a proper shared GC.

@complexmath I don't think there's such a thing as "could be generated by most GC implementations". Statistics will always be specific to the GC implementation; about the only statistic that is somewhat portable is fullCollections but even that loses its meaning in the face of a concurrent or incremental GC. I think the approach where we initialize unsupported statistics to size_t.max is reasonable enough in practice, and it still leaves the interface open to extension.

Of course we can delay merging this pull request in favor of NG discussion if you really want, but I doubt such a discussion will reach any other conclusion than I did: A portable GC statistics interface is not very realistic.

[alexrp]

So, what do we do? Anyone want to start a discussion on the NG?

[andralex]

@alexrp: will do

[alexrp]

Some wishes from the NG:

• http://golang.org/pkg/runtime/#MemStats
• http://www.haskell.org/ghc/dist/current/docs/html/libraries/base-4.6.0.0/GHC-Stats.html
• Number of collections (per generation if applicable (not to the current GC)).
• Maximum induced pause time.
• Average time of pauses.
• http://msdn.microsoft.com/en-us/library/x2tyfybc.aspx
• Mark, sweep, and page free timing.
• Allocation time including/excluding locks.