Reduce timeout to 1 second #79
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a concern about the appearance of a 5 second timeout. In my experience this is quite a high limit. https://bugs.ruby-lang.org/issues/18159#note-7. When I set the original value I didn't know what performance characteristics we would have and 5 seconds seemed like a large amount of time to recover from slowness. In practice the times we've had performance issues are due to cases where there are bugs that will either never exit #77 or due to a flaw in some subset of the logic that would otherwise take exponential time to resolve #35. Basically: If we cannot resolve a document in 1 second, there's very little chance we'll be able to figure it out in the next 4. I feel reasonably confident about dropping it down to 1. The hot path continues to be Ripper and re-parsing the document constantly. However overall it seems very performant. Also I'm keeping an eye on the `combination` logic as it might come back to be a problem for pathological cases where there are many syntax-errors rather than 1 or 2. At this time, the strategy is to wait for performance issues to occur and then tackle them. It doesn't seem to be a huge problem at the moment. Moving the performance test to a regression test as all tests must now execute under 1 second or the timeout will cause them to fail. We can consider introducing some FYI benchmarks, perhaps seeing how many IPS (Iterations Per Second) we can do on one case over 5 seconds. But not gate or fail on it. Just keep it around as an optimization target. But if that's the case, maybe it's fine to wait until do that optimization work.
schneems
force-pushed
the
schneems/drop-timeout
branch
from
026e5d7 to
2c97593
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a concern about the appearance of a 5 second timeout. In my experience this is quite a high limit.
https://bugs.ruby-lang.org/issues/18159#note-7.
When I set the original value I didn't know what performance characteristics we would have and 5 seconds seemed like a large amount of time to recover from slowness.
In practice the times we've had performance issues are due to cases where there are bugs that will either never exit #77 or due to a flaw in some subset of the logic that would otherwise take exponential time to resolve #35. Basically: If we cannot resolve a document in 1 second, there's very little chance we'll be able to figure it out in the next 4. I feel reasonably confident about dropping it down to 1.
The hot path continues to be Ripper and re-parsing the document constantly. However overall it seems very performant. Also I'm keeping an eye on the
combinationlogic as it might come back to be a problem for pathological cases where there are many syntax-errors rather than 1 or 2. At this time, the strategy is to wait for performance issues to occur and then tackle them. It doesn't seem to be a huge problem at the moment.Moving the performance test to a regression test as all tests must now execute under 1 second or the timeout will cause them to fail.
We can consider introducing some FYI benchmarks, perhaps seeing how many IPS (Iterations Per Second) we can do on one case over 5 seconds. But not gate or fail on it. Just keep it around as an optimization target. But if that's the case, maybe it's fine to wait until do that optimization work.