Skip to content

optimize goal_count (callgrind ir/call : 816 -> 126) #237

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

Open
guicho271828 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

optimize goal_count (callgrind ir/call : 816 -> 126) #237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8008590
optimize goal_count (callgrind ir/call : 816 -> 126)
guicho271828 Aug 12, 2024
eb48e68
fixup! optimize goal_count (callgrind ir/call : 816 -> 126)
guicho271828 Jan 13, 2025
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
16 changes: 12 additions & 4 deletions src/search/heuristics/goal_count_heuristic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,23 @@ GoalCountHeuristic::GoalCountHeuristic(
if (log.is_at_least_normal()) {
log << "Initializing goal count heuristic..." << endl;
}
goals.reserve(task_proxy.get_goals().size());
for (FactProxy goal : task_proxy.get_goals()) {
int var = goal.get_variable().get_id();
int val = goal.get_value();
goals.push_back({var,val});
}
}

int GoalCountHeuristic::compute_heuristic(const State &ancestor_state) {
State state = convert_ancestor_state(ancestor_state);
int unsatisfied_goal_count = 0;

for (FactProxy goal : task_proxy.get_goals()) {
const VariableProxy var = goal.get_variable();
if (state[var] != goal) {
state.unpack();
Comment thread
FlorianPommerening marked this conversation as resolved.
const vector<int> &unpacked_state = state.get_unpacked_values();
for (const auto& it : goals) {
const int& var = it.first;
const int& val = it.second;
Comment on lines +32 to +33
Copy link
Copy Markdown
Member

@FlorianPommerening FlorianPommerening Jan 13, 2025

Choose a reason for hiding this comment

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

Suggested change
const int& var = it.first;
const int& val = it.second;
int var = it.first;
int val = it.second;

We'd generally copy the values here instead of taking a reference. Should be equally fast as creating a reference.

Copy link
Copy Markdown
Member

@FlorianPommerening FlorianPommerening Jan 13, 2025

Choose a reason for hiding this comment

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

Actually, now that we have C++20, this should work as well:

for (auto[var, val] : goals) {
...
}

if (unpacked_state[var] != val) {
++unsatisfied_goal_count;
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/search/heuristics/goal_count_heuristic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

namespace goal_count_heuristic {
class GoalCountHeuristic : public Heuristic {
// caching the goals, avoiding FactProxy
std::vector<std::pair<int,int>> goals;
Copy link
Copy Markdown
Member

@FlorianPommerening FlorianPommerening Jan 13, 2025

Choose a reason for hiding this comment

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

This line could use a comment explaining that the goals are stored here for performance reasons.

Copy link
Copy Markdown
Contributor

@maltehelmert maltehelmert Jan 13, 2025

Choose a reason for hiding this comment

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

Can we run an experiment to have numbers on performance? callgrind data is useful, but usually not conclusive. The VM has its own issues when comparing to actual performance on hardware, and due to the vagaries of inlining, runtime can shift from one place to another in a way that makes it hard to draw conclusions from the profile data alone.

Copy link
Copy Markdown
Contributor Author

@guicho271828 guicho271828 Jan 13, 2025

Choose a reason for hiding this comment

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

I used the code for implementing BFWS which uses goalcount. The difference was significant. I also observed similar bottlenecks from ***Proxy while implementing other parts of BFWS code.

Copy link
Copy Markdown
Contributor Author

@guicho271828 guicho271828 Jan 13, 2025

Choose a reason for hiding this comment

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

I measured the difference in the release build on ipc instances. I lost the data though

Copy link
Copy Markdown
Contributor Author

@guicho271828 guicho271828 Jan 13, 2025

Choose a reason for hiding this comment

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

This line could use a comment explaining that the goals are stored here for performance reasons.

done

Copy link
Copy Markdown
Member

@FlorianPommerening FlorianPommerening Jan 14, 2025

Choose a reason for hiding this comment

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

I could re-run the experiment on our servers but it might take me some time to get around to it. It would help if you prepare a revision of the code that is in between this one and the main branch, where you only do the precomputation/storing of the goals, still using fact proxies in the heuristic computation. This way, we can separate the impact of this and the change avoiding the fact proxies.

protected:
virtual int compute_heuristic(const State &ancestor_state) override;
public:
Expand Down