Skip to content

chore: filtering move selector honoring the phase termination #1608

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

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

Conversation

zepfred
Copy link
Contributor

@zepfred zepfred commented May 23, 2025

This PR ensures that the FilteringMoveSelector will honor the phase termination setting. There may be use cases where a move filtering filters out all moves, resulting in the iterator analyzing a large number of moves before returning no valid upcoming selection.

It is important to note that the proposed strategy may not work for every termination configuration, and the phase will only end after the move selector bails out. Termination not based on time can cause the process to stuck during the generation of the first valid move in the phase, which may result in no move being returned after bailing out.

Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

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

I am not sure that we want to do this.
To solve a pathological corner case, we slow down almost every move selector, filtering is used very often.

IMO the solution is worse than the problem.

@zepfred
Copy link
Contributor Author

zepfred commented May 26, 2025

I am not sure that we want to do this. To solve a pathological corner case, we slow down almost every move selector, filtering is used very often.

IMO the solution is worse than the problem.

I was also concerned about the performance. In the worst-case scenario, if all moves are filtered out, there will be no drop in performance. However, if no moves are removed, there will be two calls to check the termination.

We could consider adding a triggering condition. For example, we would check that only if the filtered move count is equal to some value, such as 10% of the selector size. Also, if we can ensure the filtered selector is used in the move repository, we could remove the termination check from the decider implementation.

@zepfred zepfred force-pushed the fix/termination branch from 3f8aebd to b6e9083 Compare May 26, 2025 14:52
@zepfred zepfred force-pushed the fix/termination branch from 5c54b5e to daa309a Compare May 26, 2025 17:10
@zepfred zepfred force-pushed the fix/termination branch from daa309a to a15bd88 Compare May 26, 2025 17:13
@zepfred zepfred force-pushed the fix/termination branch from a15bd88 to 14fbcc7 Compare May 28, 2025 14:47
@zepfred zepfred force-pushed the fix/termination branch from 14fbcc7 to 5dd8fb1 Compare May 28, 2025 15:04
Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

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

Some things to consider. LGTM after resolved.

@zepfred zepfred had a problem deploying to documentation (preview) May 30, 2025 14:34 — with GitHub Actions Failure
@zepfred zepfred force-pushed the fix/termination branch from 32f78df to a3619ca Compare May 30, 2025 14:34
@zepfred zepfred deployed to internal May 30, 2025 14:35 — with GitHub Actions Active
@zepfred zepfred had a problem deploying to documentation (preview) May 30, 2025 14:35 — with GitHub Actions Failure
Copy link

Please retry analysis of this Pull-Request directly on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants