Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ECC-1602: CCSDS performance optimisation #121

Closed
wants to merge 45 commits into from
Closed

ECC-1602: CCSDS performance optimisation #121

wants to merge 45 commits into from

Conversation

joobog
Copy link
Collaborator

@joobog joobog commented May 30, 2023

With this optimisation, a significant increase in performance can be achieved, particularly for 24-bit data.

Eugen Betke and others added 20 commits August 2, 2022 18:52
ECC-1433: Reset decimal_scale_factor
a23c566
ECC-1433: Small number test in ECC-1431 enabled
c74f304
ECC-1433: Minor changes in ECC-1431
7e74575
@shahramn
ECC-1433: Test
c89e55a
ECC-1433: Remove simplified test from ECC-1431
ce58cd7
@shahramn
Merge branch 'develop' into bugfix/ECC-1433-CCSDS-encoding_small_numb…
d8007dd 
…ers_fails
@shahramn
Merge branch 'develop' into bugfix/ECC-1433-CCSDS-encoding_small_numb…
08b9fcb 
…ers_fails
@shahramn
Merge branch 'develop' into bugfix/ECC-1433-CCSDS-encoding_small_numb…
02d7965 
…ers_fails
@shahramn
Merge branch 'develop' into bugfix/ECC-1433-CCSDS-encoding_small_numb…
a087bb8 
…ers_fails
@shahramn
Merge branch 'develop' into bugfix/ECC-1433-CCSDS-encoding_small_numb…
407bb15 
…ers_fails
@shahramn
ECC-1455: Temporarily added so IFS tests can pass
6b79a1e
@shahramn
ECC-1455: Reminder to undo the change
0b2cbfc
@shahramn
Revert ECC-1455 changes
5a047a8
@shahramn
Merge branch 'develop' into bugfix/ECC-1433-CCSDS-encoding_small_numb…
b82e2c3 
…ers_fails
@joobog
ECC-1602: CCSDS performance optimisation
83e9c87
@joobog
ECC-1602: Fix reinterpret_cast<>()
e16e4ae
@joobog
ECC-1602: Refactoring: endianess_test -> is_little_endian
33ae883
@joobog
ECC-1602: Comments
b812622
@joobog
ECC-1602: Refactoring: Clean-up switch statement
1db8deb
@joobog
ECC-1602: Clean up
624c12e
@github-actions
Copy link

Private downstream CI failed.
Workflow name: private downstream ci
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5122916359.

@github-actions
Copy link

Private downstream CI succeeded.
Workflow name: private downstream ci
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5123358171.

shahramn
shahramn reviewed May 30, 2023
View reviewed changes
src/grib_accessor_class_data_ccsds_packing.cc Outdated
// ECC-1602: Performance improvement
ccsds_flags &= ~AEC_DATA_MSB; // enable little-endian
ccsds_flags &= ~AEC_DATA_3BYTE; // disable support for 3-bytes per value
unsigned short is_little_endian = 1;
Copy link
Collaborator

@shahramn shahramn May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The include file /usr/local/apps/aec/1.0.6/include/libaec.h has this comment associated with AEC_DATA_MSB:

Samples are stored with their most significant bit first. This has nothing to do with the endianness of the host. Default is LSB.

So why do we check for endianness?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why the flag is called like that, but the libaec documentation says the following: "AEC_DATA_MSB: input data is stored most significant byte first i.e. big endian. Default is little endian on all architectures."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to mention that the default value for AEC_DATA_MSB=true is stored in the definition files. Since we load this flag from a definition file, we must reset it to 0 to use libaec on a little-endian machine like ATOS HPC efficiently. In my opinion, storing machine-dependent flags like AEC_DATA_MSB and AEC_DATA_3BYTE in the definition files is unnecessary.

@shahramn shahramn added the approved-for-ci Approved to run CI on ECMWF machines label May 30, 2023
@github-actions
Copy link

github-actions bot commented Jun 2, 2023

Private downstream CI succeeded.
Workflow name: private downstream ci
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5157318731.

@github-actions
Copy link

github-actions bot commented Jun 3, 2023

Private downstream CI succeeded.
Workflow name: private downstream ci
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5163284991.

@shahramn
Copy link
Collaborator

shahramn commented Jun 3, 2023

I think we are ready to pass this branch to the IFS team for further testing

@github-actions
Copy link

Private downstream CI failed.
Workflow name: private downstream ci
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5234508935.

@github-actions
Copy link

Private downstream CI failed.
Workflow name: private downstream ci hpc
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5234525988.

@github-actions
Copy link

Private downstream CI failed.
Workflow name: private downstream ci
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5241959772.

@github-actions
Copy link

Private downstream CI failed.
Workflow name: private downstream ci hpc
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5242016941.

@github-actions
Copy link

Private downstream CI failed.
Workflow name: private downstream ci
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5242658614.

@github-actions
Copy link

Private downstream CI failed.
Workflow name: private downstream ci hpc
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5242710205.

@FussyDuck
Copy link

FussyDuck commented Jun 13, 2023 •
edited
Loading

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ joobog
✅ shahramn
❌ Eugen Betke

Eugen Betke seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions
Copy link

Private downstream CI failed.
Workflow name: private downstream ci
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/5257839172.

@shahramn
Copy link
Collaborator

I have merged this branch to develop

@shahramn shahramn closed this Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlmquintino tlmquintino tlmquintino left review comments

@shahramn shahramn shahramn left review comments

Assignees

@joobog joobog

@shahramn shahramn

Labels
approved-for-ci Approved to run CI on ECMWF machines
Projects
None yet
Milestone
No milestone
5 participants
@joobog @codecov-commenter @shahramn @FussyDuck @tlmquintino