-
Notifications
You must be signed in to change notification settings - Fork 456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add performance benchmarking scripts #917
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine ! A few comments:
- move performance-related files (scripts and filenames list to a "performance" folder in the "tests" directory)
- instead of having the coding options hard-coded in the script, it would be useful to read an external file containing the options used in different environments (see https://basecamp.com/1888053/projects/9781426/messages/62230968). The test_perf_filelist.csv would then contain the image, the number of threads, the action (compress or decompress) and an optional reference to this list of options. As we are more interested in the decoding speed, this is maybe more a nice-to-have than a mandatory feature but I think it would globally ease the benchmarking of OpenJPEG.
- I agree with your reserve on benchmarking on virtual platforms so we'll see if it gives relevant results or not (apparently the answer is no for macos). In the meantime, I suggest we evaluate the improvements based on local runs.
tools/travis-ci/run.sh
Outdated
if [ "${TRAVIS_PULL_REQUEST:-false}" == "false" ]; then | ||
REF_VERSION=v2.1.2 | ||
fi | ||
if [ ! -d ref_ojp ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strangely enough, the correct prefix or suffix for OpenJPEG is opj and not ojp ... As everything in the code follows opj, I suggest we keep this rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tests/perf_test_filelist.csv
Outdated
../data/input/nonregression/kodak_2layers_lrcp.j2c,10,4,DECOMPRESS, | ||
../data/input/conformance/p0_07.j2k,3,1,DECOMPRESS,128x128 tiles RLCP 3decomp 8layers no MCT reversible | ||
../data/input/conformance/p0_04.j2k,10,1,DECOMPRESS,precincts 128x128 irreversible | ||
../data/input/nonregression/X_4_2K_24_185_CBR_WB_000.tif,3,1,COMPRESS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a newline here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (wasn't strictly needed, but better indeed)
Done |
Agreed. I imagined that the COMPRESS string could be later extended to accept options like COMPRESS[reversible=yes,...] depending on what options we want to test. |
I've disabled making the job fail in case of performance regression, because even on Linux this is too unreliable. See https://travis-ci.org/uclouvain/openjpeg/jobs/229085228 that failed whereas previous builds (with same code) succeeded |
And run them by Travis-CI
ok. This way is possible indeed but would require a "translation" from this generic syntax to the actual kdu or opj options I guess.
ok. I think you're right : benchmark results on Travis will never be trustful. I wonder if we should not even disable the test but provide an easy way to run the script locally, as you suggested. To be discussed. |
The test can be run locally with I think we can keep it on Travis in non-failing mode as I did in the latest version of the patch, so as a hint of possible regressions or improvements, to be confirmed locally.
Yes, extract from https://travis-ci.org/uclouvain/openjpeg/jobs/229431228
|
And run them by Travis-CI
Note: I've some doubt about the reproductibility and trustability in those tests on Travis infrastructure. Initially I had tried them on OSX workers too but this seemed far too unreliable : https://travis-ci.org/rouault/openjpeg/jobs/228744947
This relates to #911