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

perf: increase default assertion solving timeout from 1s to 1m #368

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

daejunpark
Copy link
Collaborator

@daejunpark daejunpark commented Sep 21, 2024

the current default timeout for assertion solving is 1s, which is too short for most cases. as a result, many users disable the timeout altogether. this pr updates the default timeout to a more reasonable value, allowing most tests to be solved within the default time limit.

@daejunpark daejunpark force-pushed the perf/increase-default-assertion-solving-timeout branch from b82b7e6 to a9f3d11 Compare September 21, 2024 08:32
Copy link
Collaborator

@karmacoma-eth karmacoma-eth left a comment

Choose a reason for hiding this comment

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

makes sense, I like it. Only concern is that 1m maybe feels a little high, my vote would be for something like 10s-20s (enough time to give result, but not long enough that it feels stuck)

@daejunpark
Copy link
Collaborator Author

fyi, the 1m limit was chosen based on the solving time needed for those tests in our ci, on M1 pro.

i agree with your concern, but i think even 10s might feel getting stuck especially when there are many paths, unless proper communication of progress is provided. a better approach to mitigate would be to provide clearer progress updates.

@daejunpark daejunpark merged commit 3e66a60 into main Sep 23, 2024
56 checks passed
@daejunpark daejunpark deleted the perf/increase-default-assertion-solving-timeout branch September 23, 2024 22:57
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