[drafting] collector#collect performance improve - #782

Hi, performance improvements are always welcome, thanks very much.

However, in this specific case I'm not sure if we really have a problem with the current implementation. Each Sample will create a time series in the Prometheus server. So if you're saying you end up with millions of Sample instances, that would imply you are exporting millions of time series to your Prometheus server from a single application. I think this will make your Prometheus server run out-of-memory unless you put effort into scaling out your time series database.

Moreover, Java is typically very good at collecting short-lived objects. And scraping is bound by the scrape interval, which will be in the order of magnitude of 15-30 seconds, i.e. this is not called very often. I'm wondering if there is actually a measurable difference when you manage to remove this single object creation.

I would appreciate if you could provide some benchmark data showing that this actually solves a real-world problem. A benchmark would be good anyway, because with that we can compare the numbers before and after the change.

This approach would most likely result in lower memory usage (related to the use of the ThreadLocal ByteArrayOutputStream... #703) but I'm not sure it would provide any execution performance gains.

This approach would most likely result in lower memory usage (related to the use of the ThreadLocal ByteArrayOutputStream... #703) but I'm not sure it would provide any execution performance gains.

HTTPServer is a simple and easy but not the best way to expose metrics data.

In sun.net.httpserver.Request.WriteStream we can see it uses HeapByteBuffer. In HTTPServer#LocalByteArray , it uses ByteArrayOutputStream and cached in ThreadLocal(Default 5 threads). If expose 100MB metrics data once, after a fews times there will be more than 500MB heap memory cached and more than 1GB heap memory allocated(with mem_copy and resizes).

In this PR, the point is there are too much costs when collect metrics(parse Gauge,Counter,Summary... to Samples), although MetricFamilySamplesEnumeration#nextElement is a lazy operation almost can't cause OOM, but there are millions Samples objects will be generated when more than 100k meters in a system during collecting metrics. It would cause high CPU usage, high GC pressure and high heap memory usage, even OOM.
The PR is purposed to resolve these problems by write metrics data into Writer directly without generate Samples objects.

Hi, you are absolutely right about the 5 threads. I just pushed a quick PR reducing the core thread pool size to 1 (#786). Thanks for bringing this up!

For the other topic with the millions of Sample instances: I still think if you have millions of Sample objects you will end up with millions of time series and kill your Prometheus server. On the other hand, as the objects are very short-lived it might well be that the JVM has no issue cleaning them without much overhead. So having a real example and a benchmark would be good.

For the other topic with the millions of Sample instances: I still think if you have millions of Sample objects you will end up with millions of time series and kill your Prometheus server. On the other hand, as the objects are very short-lived it might well be that the JVM has no issue cleaning them without much overhead. So having a real example and a benchmark would be good.

Right, I'm trying to finish this PR and provide a simple test. It would take some time, I have to translate TextFormat#write004 into PrometheusTextFormatter#format.

Test source code: Main.txt

When collect():

When collect1():

Seems it works, please take a look @fstab . PrometheusTextFormatter only supports Gauge yet, Counter,Summary and Histogram supports to be completed.

@tjiuming May I make a suggestion...

Since it appears that you are trying to implement a visitor pattern... I would implement it as such.

public interface CollectorVisitor {

    void accept(Collector collector);
}

This would allow you to write/use a custom writer/formatter and may be a more palatable change.

@tjiuming May I make a suggestion...

Since it appears that you are trying to implement a visitor pattern... I would implement it as such.

public interface CollectorVisitor {

    void accept(Collector collector);
}

This would allow you to write/use a custom writer/formatter and may be a more palatable change.

Thanks, I also considering that provide facade and data-structure is enough, developers could expose metrics data by their own way.

Hi, just two quick remarks while you are looking into this:

• For benchmarks, I would recommend jmh. You can find some examples in the ./benchmarks/ directory.
• Please keep the API of TextFormat compatible. simpleclient_common is used by a lot of 3rd party projects.

One more remark: I saw that you are planning to introduce reflection and sun.misc.Unsafe. Please keep in mind that the core client_java metrics library is supposed to be compatible with a lot of different runtimes: Java 6, IBM Java, Native GraalVM builds, Java module system, etc. There are currently some gaps with the Java module system and I will fix it with the 1.0 release. Please don't break compatibility with Java runtimes.

@fstab @dhoard This PR will be closed, I created another PR.

@tjiuming you need to close one of the PRs. It's not clear which branch of code you are actually working with.