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

refactor: inline short functions #29216

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

IAKOBVS
Copy link
Contributor

@IAKOBVS IAKOBVS commented Jun 6, 2024

These are short wrapper functions that call other functions. For short strings, inlining them should make them faster because we're avoiding the overhead of calling the wrapper function, as if they were a macro.

@github-actions github-actions bot added the refactor changes that are not features or bugfixes label Jun 6, 2024
@dundargoc
Copy link
Member

dundargoc commented Jun 7, 2024

You'd think an optimizing compiler would easily figure out low-hanging optimizations like this automatically. Not that I'm against this change per se, but I would personally be more comfortable with some data in the form of benchmarking before changing things around. That said others in the team know C way more so I'll defer to them if they disagree with my assessment.

@clason
Copy link
Member

clason commented Jun 7, 2024

To add to that, these refactors are not free because 1. they introduce churn (make bisecting/blaming harder) and 2. introduce possible conflict with upstream Vim, which makes porting patches (even) harder.

So we tend to be not very welcoming of "I was bored and it sounded like a good idea" refactors and instead ask for objective justification. (There is a reason why we expect PR descriptions to come in a "Problem -- Solution" form -- the "problem" part is extremely important since it allows us to weigh the benefits against the listed drawbacks.)

@zeertzjq
Copy link
Member

zeertzjq commented Jun 7, 2024

And please fix the lint failures.

@justinmk justinmk added the needs:response waiting for reply from the author label Jun 7, 2024
@zeertzjq zeertzjq removed the needs:response waiting for reply from the author label Jun 8, 2024
@famiu
Copy link
Member

famiu commented Jun 12, 2024

Agree with dundar and clason here. Micro-optimizations like this should be avoided, especially if it's reasonable to assume that the compiler can handle it. Unless there is a concrete benchmark showing a significant performance increase, I am against this change

@IAKOBVS
Copy link
Contributor Author

IAKOBVS commented Jun 13, 2024

Micro-optimizations like this should be avoided, especially if it's reasonable to assume that the compiler can handle it.

If x.c includes memory.h, uses a function f from memory.h that is implemented in memory.c, and is compiled with memory.o, the compiler can not inline f in x.c because the implementation of f is invisible to the compiler when compiling x.c as memory.h only exposes the function declaration.

For example:
https://github.com/IAKOBVS/neovim-inline-demo

In the not-inlined directory, my_strlen which calls strlen is declared in f.h and defined in f.c. my_strlen is called in main.c and main.c is compiled with f.o. The objdump shows that main.c still calls my_strlen, whereas in the inlined directory, where my_strlen is defined in f.h, it shows that main.c calls strlen instead of my_strlen, and the strlen is optimized out in this case since the argument is a literal string.

@famiu
Copy link
Member

famiu commented Jun 13, 2024

Micro-optimizations like this should be avoided, especially if it's reasonable to assume that the compiler can handle it.

If x.c includes memory.h, uses a function f from memory.h that is implemented in memory.c, and is compiled with memory.o, the compiler can not inline f in x.c because the implementation of f is invisible to the compiler when compiling x.c as memory.h only exposes the function declaration.

For example:
https://github.com/IAKOBVS/neovim-inline-demo

In the not-inlined directory, my_strlen which calls strlen is declared in f.h and defined in f.c. my_strlen is called in main.c and main.c is compiled with f.o. The objdump shows that main.c still calls my_strlen, whereas in the inlined directory, where my_strlen is defined in f.h, it shows that main.c calls strlen instead of my_strlen, and the strlen is optimized out in this case since the argument is a literal string.

Okay, but what's the actual overhead of not inlining them? Are there any benchmarks that show a significant performance change?

@dundargoc
Copy link
Member

For example:
https://github.com/IAKOBVS/neovim-inline-demo

You didn't enable LTO which should take care of that. I'm not sure if it does though, I can test at home.

In any case: while there's nothing wrong with this PR (aka there's no real harm in merging this IMO) I think it's the wrong way to go to battle performance problems. You've made a a couple of performance-related PR so it seems it's something you care about. I would personally advice that you make a more targetted approach: identify features with performance problems (or look for issues with the performance label) and identify and fix the hot spots for these. I think that would be much more useful, and also make benchmarking easier as you've already identified what to benchmark (the slow feature).

@justinmk justinmk added the needs:response waiting for reply from the author label Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:response waiting for reply from the author refactor changes that are not features or bugfixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants