Issue #14599: Create performance regression test CI #14754

Lmh-java · 2024-04-02T16:49:54Z

Draft for Issue #14599

Lmh-java · 2024-04-02T21:15:26Z

.ci/baseline.benchmark

+Execution Time: 8.50 s
+============================================================
+===================== BENCHMARK SUMMARY ====================
+Average Execution Time: 8.56 s


This baseline file is generated by the shell script and is parsable by the same shell script. So whenever we want to change the baseline, we can use the same tool to generate this.

Lmh-java · 2024-04-02T21:16:24Z

config/checkstyle-non-main-files-checks.xml

@@ -19,7 +19,7 @@
  <module name="RegexpOnFilename">
    <property name="id" value="executablesLocation"/>
    <property name="folderPattern" value="[\\/].ci([\\/]|$)"/>
-    <property name="fileNamePattern" value="\.(bat|cmd|groovy|sh|md)$"/>
+    <property name="fileNamePattern" value="\.(bat|cmd|groovy|sh|md|benchmark)$"/>


Exclude benchmark files since they must are located in .ci folder but are not executable.

Lmh-java · 2024-04-02T21:17:32Z

.ci/check-performance-regression.sh

+}
+
+# parse baseline benchmark
+BASELINE=($(parse_benchmark_result "./.ci/baseline.benchmark"))


We may want to read the baseline from the master branch later after this purposal is accepted.

romani

Items

.ci/run-benchmark.sh

.ci/baseline.benchmark

.ci/run-benchmark.sh

romani

Items

.ci/check-performance-regression.sh

Lmh-java · 2024-04-04T20:39:03Z

Hi @romani, could you please take a look at the failing CI ci/circleci: no-error-xwiki.
https://app.circleci.com/pipelines/github/checkstyle/checkstyle/25030/workflows/4d5c1a92-68b6-430e-a6d4-cdd4f2716280/jobs/568220

Caused by: java.lang.RuntimeException: Cannot convert '#{env['CI'] == null ? 'ON_DEMAND' : 'ALWAYS'}' to PublishMode, valid values are 'ON_DEMAND', 'ON_FAILURE', 'ALWAYS'
    at com.gradle.maven.common.configuration.q$b.g (SourceFile:582)
    at com.gradle.maven.common.configuration.q$b.lambda$migrateOldPublishElement$8 (SourceFile:532)
    at java.util.Optional.ifPresent (Optional.java:178)

I don't think I've changed anything that is related to this CI failing, and the log doesn't really make sense to me.

romani

Ignore xwiki for now
Good job. Last item:

.github/workflows/check-performance-regression.yml

romani

Thanks a lot, ok to merge

nrmancuso

Please do:

Use SECONDS in all variable names that are time related, I've made some suggestions below.
Use consistent language for execution/run naming, I see no difference between the two in the context of this script.
Show evidence of testing (prove that this will actually fail if we have an execution time that is an outlier). In order to do this, you can make a temporary commit that rebases this PR on the commit before we merged Issue #14566: Improve lexer performance #14568

.ci/check-performance-regression.sh

config/benchmark-config.xml

config/jsoref-spellchecker/whitelist.words

Lmh-java · 2024-04-17T20:30:26Z

Thanks for your response. It might take a little bit longer to apply all these changes than usual. I am having some final exams this week. I am still working on this. Sorry for the delay.

nrmancuso · 2024-04-17T23:42:24Z

Thanks for your response. It might take a little bit longer to apply all these changes than usual. I am having some final exams this week. I am still working on this. Sorry for the delay.

No worries :) We are all busy people trying our best. Thanks for your contributions!

Lmh-java · 2024-04-22T03:36:46Z

Show evidence of testing (prove that this will actually fail if we have an execution time that is an outlier). In order to do this, you can make a temporary commit that rebases this PR on the commit before we merged #14568

~~I am working on this right now.~~

Lmh-java · 2024-04-22T19:19:37Z

@nrmancuso The CI fails in Lmh-java#6 as a test case.

I checkout 3870875 (The commit before #14568), and cherry-picked #14754 for the test.

Lmh-java · 2024-04-26T05:39:58Z

@nrmancuso, by the way, do you think it would be better if we add a line of

    if: github.repository == 'checkstyle/checkstyle'

to this workflow? so that we can make sure it only runs on our end, instead of on the developer ends.

nrmancuso · 2024-04-28T03:02:56Z

@nrmancuso, by the way, do you think it would be better if we add a line of
    if: github.repository == 'checkstyle/checkstyle'
to this workflow? so that we can make sure it only runs on our end, instead of on the developer ends.

Yeah, we have done this in most others, I agree.

Lmh-java · 2024-04-28T04:04:04Z

@nrmancuso, by the way, do you think it would be better if we add a line of
    if: github.repository == 'checkstyle/checkstyle'
to this workflow? so that we can make sure it only runs on our end, instead of on the developer ends.
Yeah, we have done this in most others, I agree.

Done

nrmancuso

Last from me:

nrmancuso · 2024-05-01T12:38:48Z

.ci/check-performance-regression.sh

+    echo "Directory $SAMPLE_PROJECT DOES NOT exist." | exit 1
+
+  # add suppressions to config file
+  sed -i '/  <!-- Filters -->/r ./config/projects-to-test/openjdk17-excluded.files' \


Please extract this filename to a variable declared above with SAMPLE_PROJECT, etc., and do anything else to make it easy/obvious to update this script to run on any java version.

@Lmh-java how did we settle on 17, instead of 21?

Please extract this filename to a variable declared above with SAMPLE_PROJECT, etc., and do anything else to make it easy/obvious to update this script to run on any java version.

Sure.

@Lmh-java how did we settle on 17, instead of 21?

I thought JDK 21 has more suppressions for latest syntax, which will consume more time. Since we are measuring the parsing time, this will make the benchmark slightly more inaccurate. So I decided to go with the JDK 17, since it contains relatively more parsable content, and we can avoid extra time consumed by a large amount of suppressions.

This is my reasoning. But if we want to go with JDK 21, it is also totally fine.

I am good with 17 for now, just curious about why we did this.

I am still working on #14779 right now. If that's merged, we can directly use that new property to skip all exceptions. That will be faster than a long list of suppressions :).

nrmancuso

Good stuff! Thanks a lot @Lmh-java this will be a big help during syntax updates.

Lmh-java force-pushed the minghao/new-ci branch from c343fb7 to 290d9fa Compare April 2, 2024 18:17

Lmh-java mentioned this pull request Apr 2, 2024

Parsing performance regression testing #14599

Closed

Lmh-java commented Apr 2, 2024

View reviewed changes

Lmh-java marked this pull request as ready for review April 2, 2024 21:17

romani requested changes Apr 3, 2024

View reviewed changes

.ci/run-benchmark.sh Outdated Show resolved Hide resolved

.ci/baseline.benchmark Outdated Show resolved Hide resolved

.ci/run-benchmark.sh Outdated Show resolved Hide resolved

Lmh-java requested a review from romani April 4, 2024 04:28

Lmh-java force-pushed the minghao/new-ci branch 2 times, most recently from d2a87dc to 8e4fd8b Compare April 4, 2024 05:24

romani requested changes Apr 4, 2024

View reviewed changes

.ci/check-performance-regression.sh Outdated Show resolved Hide resolved

.ci/check-performance-regression.sh Outdated Show resolved Hide resolved

.ci/check-performance-regression.sh Outdated Show resolved Hide resolved

Lmh-java force-pushed the minghao/new-ci branch 5 times, most recently from ae85131 to cb8ef90 Compare April 4, 2024 20:30

romani requested changes Apr 5, 2024

View reviewed changes

.github/workflows/check-performance-regression.yml Outdated Show resolved Hide resolved

Lmh-java force-pushed the minghao/new-ci branch from cb8ef90 to cfbd08a Compare April 5, 2024 05:10

Lmh-java requested a review from romani April 5, 2024 06:53

romani approved these changes Apr 5, 2024

View reviewed changes

romani assigned nrmancuso Apr 5, 2024

romani requested a review from nrmancuso April 5, 2024 12:20

nrmancuso requested changes Apr 15, 2024

View reviewed changes

Lmh-java force-pushed the minghao/new-ci branch 4 times, most recently from 4eed542 to 4a66796 Compare April 20, 2024 06:18

Lmh-java force-pushed the minghao/new-ci branch 14 times, most recently from 2f18758 to e86b762 Compare April 22, 2024 03:21

Lmh-java mentioned this pull request Apr 22, 2024

Test for performance failing Lmh-java/checkstyle#6

Open

Lmh-java requested a review from nrmancuso April 23, 2024 00:29

Lmh-java force-pushed the minghao/new-ci branch from e86b762 to 05d4265 Compare April 28, 2024 03:11

nrmancuso requested changes May 1, 2024

View reviewed changes

Lmh-java force-pushed the minghao/new-ci branch 2 times, most recently from b3e43f1 to 183bafa Compare May 1, 2024 13:55

Issue checkstyle#14599: Create performance regression test CI

a4cc442

Lmh-java force-pushed the minghao/new-ci branch from 183bafa to a4cc442 Compare May 1, 2024 14:08

Lmh-java requested a review from nrmancuso May 1, 2024 14:48

nrmancuso approved these changes May 2, 2024

View reviewed changes

nrmancuso merged commit d99bd0d into checkstyle:master May 2, 2024
115 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #14599: Create performance regression test CI #14754

Issue #14599: Create performance regression test CI #14754

Lmh-java commented Apr 2, 2024

Lmh-java Apr 2, 2024

Lmh-java Apr 2, 2024

Lmh-java Apr 2, 2024

romani left a comment

romani left a comment

Lmh-java commented Apr 4, 2024 •

edited

romani left a comment •

edited

romani left a comment

nrmancuso left a comment

Lmh-java commented Apr 17, 2024 •

edited

nrmancuso commented Apr 17, 2024

Lmh-java commented Apr 22, 2024 •

edited

Lmh-java commented Apr 22, 2024 •

edited

Lmh-java commented Apr 26, 2024

nrmancuso commented Apr 28, 2024

Lmh-java commented Apr 28, 2024

nrmancuso left a comment

nrmancuso May 1, 2024

Lmh-java May 1, 2024 •

edited

nrmancuso May 1, 2024

Lmh-java May 1, 2024

Lmh-java May 1, 2024

nrmancuso left a comment

Issue #14599: Create performance regression test CI #14754

Issue #14599: Create performance regression test CI #14754

Conversation

Lmh-java commented Apr 2, 2024

Lmh-java Apr 2, 2024

Choose a reason for hiding this comment

Lmh-java Apr 2, 2024

Choose a reason for hiding this comment

Lmh-java Apr 2, 2024

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

Lmh-java commented Apr 4, 2024 • edited

romani left a comment • edited

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

nrmancuso left a comment

Choose a reason for hiding this comment

Lmh-java commented Apr 17, 2024 • edited

nrmancuso commented Apr 17, 2024

Lmh-java commented Apr 22, 2024 • edited

Lmh-java commented Apr 22, 2024 • edited

Lmh-java commented Apr 26, 2024

nrmancuso commented Apr 28, 2024

Lmh-java commented Apr 28, 2024

nrmancuso left a comment

Choose a reason for hiding this comment

nrmancuso May 1, 2024

Choose a reason for hiding this comment

Lmh-java May 1, 2024 • edited

Choose a reason for hiding this comment

nrmancuso May 1, 2024

Choose a reason for hiding this comment

Lmh-java May 1, 2024

Choose a reason for hiding this comment

Lmh-java May 1, 2024

Choose a reason for hiding this comment

nrmancuso left a comment

Choose a reason for hiding this comment

Lmh-java commented Apr 4, 2024 •

edited

romani left a comment •

edited

Lmh-java commented Apr 17, 2024 •

edited

Lmh-java commented Apr 22, 2024 •

edited

Lmh-java commented Apr 22, 2024 •

edited

Lmh-java May 1, 2024 •

edited