-
Notifications
You must be signed in to change notification settings - Fork 128
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
[io] Increase a file size tolerance for arm builds. #1196
base: master
Are you sure you want to change the base?
Conversation
root/io/filemerger/CMakeLists.txt
Outdated
@@ -340,7 +340,7 @@ if(${compression_default} STREQUAL "zlib") | |||
if(CMAKE_SYSTEM_PROCESSOR MATCHES aarch64) | |||
ROOTTEST_ADD_TEST(simple-lz4-compr-level9 | |||
PRECMD ${ROOT_hadd_CMD} -f409 hsimple409.root hsimple.root | |||
COMMAND ${ROOT_root_CMD} -q -l -b "${CMAKE_CURRENT_SOURCE_DIR}/testSimpleFile.C(\"hsimple409.root\",25000,409,516975,5)" | |||
COMMAND ${ROOT_root_CMD} -q -l -b "${CMAKE_CURRENT_SOURCE_DIR}/testSimpleFile.C(\"hsimple409.root\",25000,409,516975,10)" |
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.
This is already in the aarch64
sections and thus the difference is from prior/different runs on arm64. Do we understand what changed?
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.
I have no idea what changed. It's the first run of the new alma9-arm runners.
git blame
says:
Author: Oksana Shadura <[email protected]>
Date: Mon Jul 8 11:47:27 2019 +0200
Adjust file size for LZ4 (409 for aarch64)
Maybe the change is 5 years of evolution of distro and lz4. I'm not sure if this test ran in the mean time.
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 you think it's safe to update the size? 7 out of 517k bytes seems like a small change.
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.
I updated to the commit message to be a bit clearer.
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.
Thanks. We should also clarify if it is used the system lz4 or the builtin version.
1351bdf
to
393f0af
Compare
Can we also note the raw number (in particular to remember whether it is a slight increase or slight decrease in size :) )? Thanks. |
Sure, in the commit or in the CMakeLists? Can I ask, though, if changes on the level of 0.0052% are relevant? Shouldn't we be more forgiving, and only start looking when we reach something like the per-mille level? |
Commit log ;)
They can be. Changes that are 'problems' that can lead to small changes includes unintentional but consistent changes to the I/O (i.e. content has changed/increased but everything works thanks to schema evolution) .... And technically the unintentional change might be a bug/random-behavior that is being seen only rare platforms - so if the change was not so small and only in one file, the right process would have been to:
i.e. this PR assumes the change is only due to a change in the compression algorithm and is not related to a architecture specific flaw in the ROOT code. |
With alma9 and its system lz4 1.9.3, the filesize of the 409 test increased. 516975 --> 516703 byes. The previous value is from 2019 (unknown distro). Increased the tolerance to 30 now.
Thanks. |
On arm, a filesize comes out 27 bytes different from
x86the 2019 arm values, so the tolerance had to be increased a bit.This is a prerequisite for #root-project/root-16526