Skip to content

arg: slightly reduce compilation time #17324

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

Closed
wants to merge 1 commit into from
+14 −14
Closed

arg: slightly reduce compilation time #17324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions common/arg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,25 +60,25 @@ static std::string read_file(const std::string & fname) {
return content;
}

common_arg & common_arg::set_examples(std::initializer_list<enum llama_example> examples) {
common_arg && common_arg::set_examples(std::initializer_list<enum llama_example> examples) {
this->examples = examples;
return *this;
return std::move(*this);
}

common_arg & common_arg::set_excludes(std::initializer_list<enum llama_example> excludes) {
common_arg && common_arg::set_excludes(std::initializer_list<enum llama_example> excludes) {
this->excludes = excludes;
return *this;
return std::move(*this);
}

common_arg & common_arg::set_env(const char * env) {
common_arg && common_arg::set_env(const char * env) {
help = help + "\n(env: " + env + ")";
this->env = env;
return *this;
return std::move(*this);
}

common_arg & common_arg::set_sparam() {
common_arg && common_arg::set_sparam() {
is_sparam = true;
return *this;
return std::move(*this);
}

bool common_arg::in_example(enum llama_example ex) {
Expand Down Expand Up @@ -717,9 +717,9 @@ common_params_context common_params_parser_init(common_params & params, llama_ex
* - if LLAMA_EXAMPLE_* is set (other than COMMON), we only show the option in the corresponding example
* - if both {LLAMA_EXAMPLE_COMMON, LLAMA_EXAMPLE_*,} are set, we will prioritize the LLAMA_EXAMPLE_* matching current example
*/
auto add_opt = [&](common_arg arg) {
auto add_opt = [&](common_arg && arg) {
Copy link
Collaborator

@Acly Acly Nov 19, 2025

Choose a reason for hiding this comment

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

IMO this is the only change that is needed, and which actually avoids copying.

All the other changes shouldn't make a difference.

Copy link
Collaborator

@Acly Acly Nov 19, 2025

Choose a reason for hiding this comment

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

Sorry I just realized the change makes it an R-value.

I think, just change it to common_arg & (regular L-value reference). It still allows moving, should guarantee no copies, and no other changes needed. It's 10% faster for me compared to master.

Copy link
Collaborator Author

@ngxson ngxson Nov 19, 2025

Choose a reason for hiding this comment

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

how it supposes to compile without other changes?

......./common/arg.cpp:771:5: error: no matching function for call to object of type '(lambda at ......./common/arg.cpp:720:20)'
  771 |     add_opt(common_arg(
      |     ^~~~~~~
......./common/arg.cpp:720:20: note: candidate function not viable: expects an rvalue for 1st argument
  720 |     auto add_opt = [&](common_arg && arg) {
      |                    ^   ~~~~~~~~~~~~~~~~~

Copy link
Collaborator Author

@ngxson ngxson Nov 19, 2025

Choose a reason for hiding this comment

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

common_arg & doesn't work either, please test before comment

Copy link
Collaborator

@Acly Acly Nov 19, 2025

Choose a reason for hiding this comment

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

My bad, I tested it with MSVC and it works, but it's technically not correct (to bind temporary to l-value ref).

Hm I really wanted there to be a simpler solution, because the way the functions work now they implicitly transform L-values into R-values, which I think is not nice.

To illustrate the problem:

common_arg arg;
std::vector<common_arg> vec;
vec.push_back(arg.set_sparam());
// use arg here is UB, because set_sparam made it possible to be moved into the vector, which is not obvious

To avoid that, the functions can be declared like this:

common_arg && set_sparam() &&;  // can only be callsed if it is a R-value already

but then you typically have to supply L-value overloads too, and that's a lot of boilerplate.

Copy link
Collaborator Author

@ngxson ngxson Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we need a l-value overload as we never use that case in practice. The current infrastructure of arg.cpp is currently built on many assumptions like that, mainly for syntactic sugar.

But I'd agree that we're maybe going for too many boilerplates and hacks for too little improvement, I think the current PR is not worth merging as @ggerganov suggested above. Closing it now and maybe revisit it later if there are better ideas.

if ((arg.in_example(ex) || arg.in_example(LLAMA_EXAMPLE_COMMON)) && !arg.is_exclude(ex)) {
ctx_arg.options.push_back(std::move(arg));
ctx_arg.options.emplace_back(std::move(arg));
}
};

Expand Down
8 changes: 4 additions & 4 deletions common/arg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ struct common_arg {
void (*handler)(common_params & params, const std::string &, const std::string &)
) : args(args), value_hint(value_hint), value_hint_2(value_hint_2), help(help), handler_str_str(handler) {}

common_arg & set_examples(std::initializer_list<enum llama_example> examples);
common_arg & set_excludes(std::initializer_list<enum llama_example> excludes);
common_arg & set_env(const char * env);
common_arg & set_sparam();
common_arg && set_examples(std::initializer_list<enum llama_example> examples);
common_arg && set_excludes(std::initializer_list<enum llama_example> excludes);
common_arg && set_env(const char * env);
common_arg && set_sparam();
bool in_example(enum llama_example ex);
bool is_exclude(enum llama_example ex);
bool get_value_from_env(std::string & output) const;
Expand Down
Loading