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

Audit use of const& in interfaces #874

Open
lifflander opened this issue Jun 21, 2020 · 3 comments · May be fixed by #1875
Open

Audit use of const& in interfaces #874

lifflander opened this issue Jun 21, 2020 · 3 comments · May be fixed by #1875
Labels
pri: low type: task
Projects
VT

Comments

@lifflander
Copy link
Collaborator

What Needs to be Done?

VT is very aggressive with using const& to pass small types...i.e. basic types around. It's extra overhead reading the code and might even slow it down because the compiler has to preserve the memory address. Switch these to just const?
@lifflander lifflander mentioned this issue Jun 21, 2020
Merged
@pnstickne
Copy link
Contributor

pnstickne commented Jun 23, 2020 •
edited

Should not use const& on any types <= 64bits, in general. Probably no loss in pref up to ~128bits. Numbers online indicate ~12bytes being some trade-off. YMMV on many factors. Compilers may also elect to ignore the const& in certain cases.

MsgPtr etc. is small exception as it allows bypassing ref count management, eg.

@lifflander lifflander added this to Unassigned in VT Sep 22, 2020
@cz4rs
Copy link
Contributor

cz4rs commented Sep 6, 2021

If possible, this should be detected automatically in CI.
Also consider compiler optimizations.

@jstrzebonski jstrzebonski mentioned this issue May 9, 2022
Merged
@jstrzebonski
Copy link
Contributor

I wonder if we could use clang-tidy for that? Or at least for reporting the occurrences of such const refs.

@jstrzebonski jstrzebonski self-assigned this May 10, 2022
@jstrzebonski jstrzebonski moved this from Unassigned to To do in VT May 10, 2022
@jstrzebonski jstrzebonski moved this from To do to In progress in VT Jun 2, 2022
@nmm0 nmm0 mentioned this issue Jun 7, 2022
Open
jstrzebonski pushed a commit that referenced this issue Jul 19, 2022
#874 add docker to run custom clang-tidy version
21f42e5
@jstrzebonski jstrzebonski linked a pull request Jul 19, 2022 that will close this issue
Draft
jstrzebonski pushed a commit that referenced this issue Jul 20, 2022
#874 run clang-tidy workflow
e340569
jstrzebonski pushed a commit that referenced this issue Jul 30, 2022
#874 add mist-redundant-const-refs check
7b23f22
jstrzebonski pushed a commit that referenced this issue Nov 9, 2022
#874 add docker to run custom clang-tidy version
8295436
jstrzebonski pushed a commit that referenced this issue Nov 9, 2022
#874 run clang-tidy workflow
77e7a70
jstrzebonski pushed a commit that referenced this issue Nov 9, 2022
#874 add mist-redundant-const-refs check
71b5368
jstrzebonski pushed a commit that referenced this issue Nov 9, 2022
#874 add mist-redundant-const-refs check
30d6b95
@jstrzebonski jstrzebonski mentioned this issue Nov 16, 2022
Open
@jstrzebonski jstrzebonski removed their assignment Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
pri: low type: task
Projects
VT
  
In progress
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

#874: add docker to run custom clang-tidy version DARMA-tasking/vt
4 participants
@pnstickne @lifflander @cz4rs @jstrzebonski