What about method3? Should it's duration also be changed? (maybe 3 or 7 milliseconds)

What about method3? Should it's duration also be changed? (maybe 3 or 7 milliseconds)

Sure let me re-run the tests with the new duration to make sure everything is working as expected

What about method3? Should it's duration also be changed? (maybe 3 or 7 milliseconds)

@apangin neither values of 7 & 3 are stable with the current iteration counts, I would recommend keeping it as 10

What does it mean it's not stable? And how does 10ms duration make it "more stable", given that it correlates with the sampling interval?

What does it mean it's not stable? And how does 10ms duration make it "more stable", given that it correlates with the sampling interval?

@apangin I test the stability of a given option by simply re-running the same test 100s of times, then measure how likely it's to fail

In the case of using 7,3ms the failure rate was quite high,
Both never managed to go a full 100 runs without failing

As for why 10ms is more stable, it would require an actual full analysis of how the CPU executes the java byte code & create some models to predict when samples would happen, & I honestly didn't see the value of going through that given that the current numbers were observed to be quite stable on multiple systems

Are these values tuned to a specific CPU type/size?

Are these values tuned to a specific CPU type/size?

I tried to run this changes across multiple CPU types & sizes & didn't observe any instabilities on the configuration I ran against

neither values of 7 & 3 are stable with the current iteration counts, I would recommend keeping it as 10

This is contrary to the declared intention of this PR, which is supposed to reduce correlation with the sampling interval. You are saying that the test is more stable when method3 runs for 10ms, which is exactly the sampling interval. This sounds counterintuitive.

I don't mind skipping time consuming analysis if test behavior matches expectation, but here I see the opposite. I've always said that the purpose of a test is not to "pass", but to validate assumptions. My assumption here is, if a test runs 3 methods for a comparable amount of time each, all 3 methods should be visible in a profile. Where am I wrong? What makes you sure that it's a test issue rather than a profiler bug or a JVM bug?

This is contrary to the declared intention of this PR, which is supposed to reduce correlation with the sampling interval

The intention here is reduce possible synchronization of all 3 methods together as a single unit with the sampling interval.
Not to approach each method individually

I would describe as reducing the chance that all 3 methods come to an agreement to actively prevent one of them from being sampled from the profiler by coordinating their run time.

My assumption here is, if a test runs 3 methods for a comparable amount of time each, all 3 methods should be visible in a profile

This is a fair assumption but kinda goes against what the test actually runs, as the methods do not strictly run for comparable time to one another in a controlled manner,

Method1 & 2 can take different times to complete depending on the system they run on (CPU resources, JDK version & optimizations)
While method3 is written with a strict minimum run time (10ms Currently)

For example let's assume we change method3 to run for 3ms & method2/1 have a combined run time of 7ms on a random system. (total run time will be 3 + 7 = 10ms = sampling duration)
We would end-up with all 3 methods (as a single unit) having a synchronization against the 10ms sampling time which might cause sampling issues.

What makes you sure that it's a test issue rather than a profiler bug or a JVM bug

If it's a JVM bug it would have been viewable in the generated byte code, (For example one method is completely optimized out)
As for If it's a AGCT bug, that's quite unlikely.

Technically speaking it's a limitation of the profiler due to the realities of how sampling is collected.
=> The profiler collects the samples via a time based approach (for CPU samples), so it's possible for that approach to produce some biased sampling results for strictly periodic/consistent work loads as we do here in this test.

While this is is indeed a limitation of the profiler, I wouldn't personally consider it a bug per say

Now, regarding this test,
It does expose a real limitation of the profiler when profiling strictly periodic/consistent work loads
This issue is indeed true & can happen on all profiling engines/modes of the profiler
=> For example, nativemem tracing with interval of 10000bytes, & having two methods back to back running in a loop with methodA allocating 9999bytes & methodB allocating 1 byte (MethodA will never be sampled)

If this is to be considered a bug/limitation that needs to be fixed, that would need to be discussed in depth & don't believe that this PR is the proper place to handle that discussion.

For this test in particular to reduce the chance of failures that may happen due to possible biased sampling of periodic/consistent work loads we should re-write this test to no longer be strictly periodic/consistent,
In other words introduce some randomness to shift the run time for each method at each iteration

IMO that would make it a better smoke test for async-profiler, as most real applications don't have strictly periodic/consistent loads,
& would reduce the possibility of time bias for the whole work load as a single unit.

What are you thoughts about this?