OpenLiberty / open-liberty

Open Liberty is a highly composable, fast to start, dynamic application server runtime environment
https://openliberty.io
Eclipse Public License 2.0
1.13k stars 575 forks source link

InstantOn Performance: Possible improvement with mpMetrics/monitor-1.0 #28446

Open jdmcclur opened 2 weeks ago

jdmcclur commented 2 weeks ago

When the mpMetrics-x.y feature is enabled (or a microProfile-x.y convenience feature), monitor-1.0 is also enabled to help gather some of the metrics needed for REST endpoints. I usually run with this filter because I don't need the other filters enabled by default with monitor-1.0.

<monitor filter="REST"/>

Some of the other monitor filters (ServletStats, SessionStats, ThreadPoolStats?) use Probes and monitor-1.0 adds instrumentation/tranformers to make these probes work. This instrumentation adds time to startup/first request time.

https://github.com/OpenLiberty/open-liberty/blob/0093dc55b024735ae54700416814bb223216b753/dev/com.ibm.ws.monitor/src/com/ibm/ws/monitor/internal/ProbeManagerImpl.java#L205

When using InstantOn, if I manually disable these probes, first request time improves by 17%. So I am wondering if we can disable the probes if we're not using them or have a property to disable them?

donbourne commented 2 days ago

@jdmcclur , to clarify, are you saying that some of the monitor-1.0 components require bytecode injection (BCI) and some don't, but today we do the BCI for all when monitor-1.0 is enabled, even if the list of selected components may not require any BCI?

jhanders34 commented 2 days ago

If we can dynamically enable different filter settings I am guessing that the probing needs to happen so that if they enable Servlet dynamically that we start getting servlet stats possibly?

NottyCode commented 2 days ago

@jdmcclur I'm not the expert here, but this bug report raises a bunch of questions that I don't quite understand.

I think you are saying you have configured the filters to disable all monitoring data except the REST monitoring data and that doesn't require the probes to be setup. I think you are therefore saying that the 17% cost is doing setup for monitoring data that we do not record. I might have misunderstood this.

I don't think it is common that someone configures monitor like this, so while we could save 17% in a performance benchmark I would be concerned about the value of special casing when only rest is configured since that would be an improvement likely only seen by our benchmarks, not actual users.

The question also suggests to me that we are doing probe introspection during the checkpoint restore phase of runtime, but I think this is a case where we should be doing that during checkpoint creation. So have you looked into moving the probe instrumentation into the checkpoint creation phase instead of removing it to see what effect that would have?

jdmcclur commented 2 days ago

@donbourne - yes, we do the probe introspection automatically if monitor-1.0 is enabled.

I am still not sure 100% what is all is going on here as I am bit limited in profiling in InstantOn envs. But this is my educated guess.

I don't think it is common that someone configures monitor like this.

I agree most customers don't do this, but we have it documented for performance to do this somewhere. Do customers who enable microProfile-x.y or mpMetrics-x.y even know they are getting monitor-1.0? They probably don't care about most of the monitor-1.0 function, but get the startup and throughput overhead. Why punish them for it? Or maybe monitor-1.0 is providing more than I know about?