TimeWindowMax#record causes allocation

[agusevas]

Some methods in TimeWindowMax class produce garbage because of usage of capturing lambdas. For example:

    public void record(double sample, TimeUnit timeUnit) {
        record(() -> (long) TimeUtils.convert(sample, timeUnit, TimeUnit.NANOSECONDS));
    }

    private void record(LongSupplier sampleSupplier) {
        rotate();
        long sample = sampleSupplier.getAsLong();
        for (AtomicLong max : ringBuffer) {
            updateMax(max, sample);
        }
    }

Every time the record(double sample, TimeUnit timeUnit) is called, a new lambda instance will be created. See example memory allocation profiler stack trace:

Environment

• Micrometer version: 1.8.5
• Micrometer registry: prometheus
• OS: macOS
• Java version: 17

To Reproduce
How to reproduce the bug:

import java.util.concurrent.TimeUnit;

import org.junit.jupiter.api.Test;

import io.micrometer.core.instrument.Timer;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;

public class TimeWindowMaxMemoryAllocation {

	@Test
	void timeWindowMax() {
		SimpleMeterRegistry meterRegistry = new SimpleMeterRegistry();
		Timer timer = Timer.builder("timer").register(meterRegistry);
		for (int i = 0; i < 1_000_000; i++) {
			timer.record(1, TimeUnit.MILLISECONDS);
		}
	}
}

Run the test using memory allocation profiler. Sample result:

Expected behavior
Garbage creation is reduced in TimeWindowMax class.

Additional context
See https://www.infoq.com/articles/Java-8-Lambdas-A-Peek-Under-the-Hood/ for more information about the capturing lambdas.

[shakuzen]

Looks like this was introduced with #1641. I'm wondering why we used a supplier at all instead of directly taking a long parameter. @izeye do you remember?

[izeye]

@shakuzen I can't remember exactly, but it seems to have been done that way as part of refactoring to reduce duplication. It'd be better to be reverted if it causes any performance downsides.

[shakuzen]

I did quite a bit of investigation for this. It's a bit difficult to be certain about things because behavior can vary depending on the compiler and other factors. I do not see any garbage from the lambda in a heapdump, but I do see the same allocation you are seeing with the profiler. I think it's likely the allocation is happening on the stack due to escape analysis. If I add a breakpoint on the lambda and pause execution in it, then I can see garbage in a heapdump for the lambda instance. Since we don't need to use a capturing lambda here, and this is the hot path for recording, we should avoid the allocation.
As a separate issue, it would be nice if we can add tests and assertions about allocations to avoid this kind of problem.

[shakuzen]

@agusevas Thank you for reporting the issue. The fix is available in snapshots now (1.8.7-SNAPSHOT, 1.9.1-SNAPSHOT). I did not see the allocation in the memory profiler with these changes. Let us know if you still see allocation happening.

[agusevas]

@shakuzen Thank you for the fix. I have tested the 1.8.7-SNAPSHOT version locally. The allocation issue is fixed now.