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

Some changes in preparation for the shrinker ir migration #3903

Merged
merged 35 commits into from
Mar 4, 2024

Conversation

tybug
Copy link
Member

@tybug tybug commented Feb 28, 2024

(mostly) split from #3899, as promised.

This is a bunch of small independent changes bundled together, plus a big one in 1e76ce2. I've also included two small bug fixes I ran into (2b05fb8, 251cf43). It may be easiest to review this commit-by-commit.

I'm not super happy with the api design of modifying/copying IRTrees. I expect to iterate on this pretty quickly as I migrate future shrinking passes, so I haven't put too much effort into trying to get it perfect the first time. That said, if you have strong opinions on how this kind of thing should be designed, feel free to tear the current implementation apart!

Conversations of note:

@tybug tybug requested a review from Zac-HD as a code owner February 28, 2024 20:37
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Very nice! I've just pushed a tiny increment more space-saving by keying off the integer hash of the sorted tuple-key. Unfortunatly WeakValueDictionary doesn't support dicts as values, or I'd have used that instead of the LRUReusedCache...

If you're happy with this, feel free to merge!

Comment on lines +224 to +225
# nans with a sign opposite of both bounds previously gave us trouble
# trying to use float clampers that didn't exist when drawing.
Copy link
Member

Choose a reason for hiding this comment

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

oooooohhhhh, that's what was causing the flakes. nicely cursed!

@tybug
Copy link
Member Author

tybug commented Mar 4, 2024

hash(tuple(sorted(key))) is actually going to cause some flakes because we're bypassing the identity fallback; hash(18446744073709551615) == hash(7) and now we're returning the same cached object for different kwargs. I had the same initial implementation and should have left an inline comment; that's my bad!

@Zac-HD
Copy link
Member

Zac-HD commented Mar 4, 2024

Heh, just noticed that and reverted 😅

@tybug
Copy link
Member Author

tybug commented Mar 4, 2024

d'oh, test_rewrite_filter_chains_with_some_unhandled flaking. Pretty sure I've seen that one before. Let's just health-check it away?

@Zac-HD Zac-HD enabled auto-merge March 4, 2024 01:52
@Zac-HD Zac-HD merged commit fe92cff into HypothesisWorks:master Mar 4, 2024
48 checks passed
@tybug tybug deleted the ir-shrinker-preparation branch March 4, 2024 05:15
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.

None yet

2 participants