Review request JMC-6512: for Competing CPU load rule should take overall load into account

[Florian David]

Le jeu. 27 juin 2019 à 23:38, Jie Kang <jkang at redhat.com> a écrit :

> Hi Florian,
>
> Hi Jie,

Thanks for the review. Please find the new one taking into account your
feedback here:
http://cr.openjdk.java.net/~hirt/fdavid/JMC-6512/webrev.1/


> Changes look mostly good.
>
> # I have some suggestions and comments below:
>
> * The Messages also contain:
>
> CompareCpuRule_INFO_LIMIT=Competing CPU usage limit
> CompareCpuRule_INFO_LIMIT_LONG=The amount of CPU used by other
> processes needed to trigger an info notice
> CompareCpuRule_WARNING_LIMIT=Competing CPU usage warning limit
> CompareCpuRule_WARNING_LIMIT_LONG=The amount of CPU used by other
> processes needed to trigger a warning
>
> I think this patch should also alter these descriptions to reflect the
> new calculation behind the rule.  To be honest I'm not sure how to
> describe the new meaning succinctly in words, otherwise I'd provide a
> suggestion, sorry;
>
> I have updated these messages:

CompareCpuRule_TEXT_INFO_LONG=The application performance can be affected
when the machine is under heavy load and there are other processes that use
CPU or other resources on the same computer. To profile representatively or
get higher throughput, shut down other resource intensive processes running
on the machine.
...
CompareCpuRule_TEXT_MESSAGE=An average CPU load of {2} was caused by other
processes during {0} starting at {1}.
CompareCpuRule_INFO_LIMIT=Competing CPU usage info limit
CompareCpuRule_INFO_LIMIT_LONG=The amount of CPU used by other processes
times the overall CPU usage needed to trigger an info notice
CompareCpuRule_WARNING_LIMIT=Competing CPU usage warning limit

I tried to keep these messages simple and not overwhelming the user with
lot of details.
However, if you think it should be even more detailed, I can be more
precise.


> # The following comments could be addressed in different Jira issues, if
> at all:
>
> * The SpanToolkit is used twice on the items now, once for the max and
> once for the ratio. Could the underlying iterations over the items be
> combined? I understand they calculate different spans, one of which is
> used in the message while the other is used for the score. Could they
> be aligned?
>
> * It's curious that the SpanToolkit is given the warning limit whereas
> the long message is generated if the score is above the info limit. As
> well, the warning limit is used to determine if spans get combined
> while the end result used for the score process is the longest span,
> regardless of it's value's relation to the warning limit. Should it be
> the longest span that's above the warning limit, if possible,
> otherwise the longest span? I guess it depends on if the initial spans
> vary in length or not. I'm not sure how to describe it but something
> feels strange here.
>
> I agree, these comments are relevant. However I'm not yet a contributor
thus I don't have rights to create Jira tickets :( How can we address that ?

For the second point, I also believe there is something fishy in this code
about using the warning limit instead of the info limit is incorrect. I'm
not
mastering spans already but passing warning limit to *getMaxSpanLimit*
should exclude span below this limit, thus only triggering the rule if
score
is above warning limit only and never of it's between info and warning
limits.
Something is wrong in this workflow.

# The following is just a question for the intended design:
>
> * After an initial read of these changes, I would expect that if I set
> a warning limit of say 0.4, then a warning would be triggered if there
> is a longest span where the CPU usage of other processes was 40% and
> the total CPU usage was 100% I.e. the range is like [1]? Is that
> right?
>
> [1]
> x*y<LIMIT, 0<=x<=1, 0<=y<=1, x<y
> x: CPU Usage of Other Processes over the longest span of time
> y: CPU Usage of All Processes over longest span of time
>
> Yes that's exactly the computation being done. The core idea is that when
machine CPU is low, it's meaningless to trigger the rule since there is
still enough
CPU to run other processes. The multiplication by 'y' decreases the score
in this case.

Regards,
Florian


>
> Regards,
>
>
> On Thu, Jun 27, 2019 at 5:34 AM Florian David <florian.david at gmail.com>
> wrote:
> >
> > Hi all,
> >
> > Please review this fix for the CPU load rule.
> >
> > JIRA: https://bugs.openjdk.java.net/browse/JMC-6512
> > Webrev: http://cr.openjdk.java.net/~hirt/fdavid/JMC-6512/webrev.0/
> >
> > Regards,
> > Florian
>