Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[cmd] Improve isScheduled to be more performant when checking a single command #7096

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

oh-yes-0-fps
Copy link
Contributor

@oh-yes-0-fps oh-yes-0-fps commented Sep 19, 2024

No description provided.

@oh-yes-0-fps oh-yes-0-fps requested a review from a team as a code owner September 19, 2024 03:09
Copy link
Contributor

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

Comment on lines 508 to 513
public boolean isScheduled(Command command, Command... commands) {
if (commands.length == 0) {
return m_scheduledCommands.contains(command);
} else {
return m_scheduledCommands.contains(command) && m_scheduledCommands.containsAll(Set.of(commands));
}
Copy link
Member

@SamCarlberg SamCarlberg Sep 19, 2024

Choose a reason for hiding this comment

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

A simple loop over the array would be fine (this also avoids the Set allocation)

public boolean isScheduled(Command... commands) {
  for (var command : commands) {
    if (!m_scheduledCommands.contains(command)) {
      return false;
    }
  }

  return true;
}  

Wrapping in Set.of is useful only if the same command object is passed in more than once, which should be rare or even nonexistent.

Note that a variadic argument will always create a new array, even if no objects are passed in; ie, isScheduled() effectively compiles to isScheduled(new Command[0]), so this approach doesn't prevent array allocations. If you want to avoid array object allocations, overloads for some number of predetermined parameter counts would be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

That loop would make it match the C++ version too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it worth doing the predetermined overloads? multi-command schedule checks are already uncommon

Copy link
Member

Choose a reason for hiding this comment

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

A single-command overload would probably be fine with varargs as a fallback

@oh-yes-0-fps
Copy link
Contributor Author

/format

@SamCarlberg
Copy link
Member

@oh-yes-0-fps you'll need to update with main to resolve the failing checks

@PeterJohnson PeterJohnson merged commit 180349b into wpilibsuite:main Sep 23, 2024
33 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants