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.

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

Removed const references for simple types and structures less 16 bytes #11294

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GermanAizek
Copy link
Contributor

@GermanAizek GermanAizek commented Jan 18, 2025

As a minor improvement, std::string is made as param by constant reference, and probs_iterator tmp has become constant iterator.

In general, passing simple types and objects less than 16 bytes by reference makes optimization worse by modern compiler because it strictly follows rules programmer. I don't know if this will affect -O2 and even more so -O3.

Reference: https://stackoverflow.com/a/3314034

@github-actions github-actions bot added the ggml changes relating to the ggml tensor library for machine learning label Jan 18, 2025
@GermanAizek
Copy link
Contributor Author

@JohannesGaessler, I'm sorry, I accidentally called you to do a code review.

Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

Keep only the changes in common/arg.cpp and ggml/src/gguf.cpp. The others are either not improvements or would just create conflicts with more important changes.

src/unicode.cpp Outdated Show resolved Hide resolved
src/llama-sampling.cpp Outdated Show resolved Hide resolved
src/llama-sampling.cpp Outdated Show resolved Hide resolved
src/llama-kv-cache.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ggml changes relating to the ggml tensor library for machine learning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants