Reintroduce Shrink.list_spine optimization #283
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
jmid
force-pushed
the
reintroduce-list-shrinker-opt
branch
from
89852fb to
1b3e1a0
Compare
Collaborator
Author
|
Rebased on |
Contributor
|
Another possibility would be: if (try x <> y with Invalid_argument _ -> x != y) then ... |
Collaborator
Author
|
Ha! That's an interesting suggestion... To better understand, I tried the suggestion and reran the test suite to spot which other optimizations it would pick up and report as expect-test and unit-test failures. The result was a none however. This indicates a test suite short-coming, as it should make a difference when shrinking heap allocated data, say I also reran the shrinker benchmark (details in unfolding below):
Overall
iteration seed 1234 iteration seed 8743 iteration seed 6789 total
Shrink test name Q1/s #succ/#att Q2/s #succ/#att Q1/s #succ/#att Q2/s #succ/#att Q1/s #succ/#att Q2/s #succ/#att Q1/s Q2/s
< bind list_size constant 0.000 52/230 0.000 12/25 0.000 49/227 0.000 9/19 0.000 43/192 0.000 10/19 0.000 0.000
< lists shorter than 10 0.000 49/315 0.000 15/24 0.000 50/315 0.000 17/34 0.000 36/236 0.000 13/28 0.000 0.000
< lists shorter than 432 0.184 1738/20737 0.738 413/448 0.100 1667/19905 0.312 419/461 0.185 1712/20417 0.686 422/471 0.470 1.736
---
> bind list_size constant 0.000 52/207 0.000 12/25 0.000 49/204 0.000 9/19 0.000 43/171 0.000 10/19 0.000 0.000
> lists shorter than 10 0.000 49/279 0.000 15/24 0.000 50/282 0.000 17/34 0.000 36/210 0.000 13/28 0.000 0.000
> lists shorter than 432 0.178 1738/19018 0.739 413/448 0.104 1667/18254 0.287 419/461 0.187 1712/18727 0.661 422/471 0.469 1.687As such, I lean towards going with the proposed change for now as it gets us a nice improvement on some list shrinking situations at an affordable complexity. Full benchmark detailsmainthis PRabove suggestion |
Collaborator
Author
|
On second thought, there's something nice about the shrinker avoiding redundant candidates equally well on unboxed and boxed data types without an end-user having to worry about the difference. Generally I would expect a property to be more costly than a |
Collaborator
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This little PR reintroduces a small optimization to
Shrink.list_spinethat had to be rolled back in #280.The reason was that the polymorphic difference (inequality?) operator
<>would fail when comparing list elements with function values:Kudos to @shym for suggesting a simple fix: use an address comparison
!=instead!On the one hand this
test_list_spine_compareOn the other hand, it doesn't identify as many equalities since, e.g., identical strings don't necessarily live at the same address 🤷