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

Allow Relation<Key> to join with Variable<(Key, Value)> directly #36

Merged
merged 2 commits into from Aug 30, 2021

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Aug 12, 2021

Polonius has to create new intermediate variables with types like Variable<(Key, ())> from Relation<Key> so that they can be joined with variables like Variable<(Key, Value)>. This happens once in the naive variant (see below) and twice in the optimized one. This isn't actually necessary, since we can legally transmute a Relation<Key> to a Relation<(Key, ())> according to the unsafe-code guidelines. This saves a bit of memory and some time copying tuples around, although these relations aren't very large on any of the inputs bundled with Polonius. Mostly, it makes the code clearer by removing the number of variables used solely for re-indexing.

To do this safely, I had to change how Relation implements JoinInput. This actually fixes a bug in Relation::from_antijoin (which we don't use), but it's not obviously correct. See the second commit for details.

I changed the signature of JoinInput so its semantics could remain the same. Relation::from_antijoin is still broken, however.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Aug 14, 2021

Whoops, 25c7774 is actually a terrible idea. Now when a Relation is used as a JoinInput, its tuples are joined with both the stable and the recent tuples of the variable (instead of just the recent ones). Unfortunately, StableTuples cannot (soundly) be transmuted in the necessary fashion. We'll have to change the API to make this work.

@ecstatic-morse ecstatic-morse marked this pull request as draft August 14, 2021 20:01
@ecstatic-morse ecstatic-morse marked this pull request as ready for review August 14, 2021 21:07
@lqd
Copy link
Member

lqd commented Aug 15, 2021

This looks good to me. And with it, rust-lang/polonius#178 should actually improve on clap now, right ?

I do wonder what to do about Relation::from_antijoin. Should we remove it until it's fixed ? We can at the very least create an issue describing the problem so that we don't forget.

Thankfully these Relation helpers are not really important compared to doing the actual joins on the Variables.

@ecstatic-morse
Copy link
Contributor Author

Yep! I'm glad I tested that PR first XD.

I think we can just refactor the helper function so it works on a Relation instead of a JoinInput. I'll open an issue with mentoring instructions, since this isn't too urgent.

This case occurred in `Polonius`, where we had to create a separate
`Relation<(T, ())>` to make things work.

Implementing this required a change to the interface of `JoinInput`,
since we are not allowed to assume that a `Relation<T>` and a
`Relation<(T, ())>` are layout compatible. Now, `stable` takes a
callback instead of returning a slice directly.
Layout compatibility guarantees for `T` do not extend to types
containing `T` (like `&[T]`), so use `from_raw_parts` instead of
`mem::transmute`.
@lqd
Copy link
Member

lqd commented Aug 30, 2021

@ecstatic-morse btw do you know whether miri complained with the previous transmute ?

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Aug 30, 2021

It won't complain on either of them. From the Miri docs:

If the program relies on unspecified details of how data is laid out, it will still run fine in Miri -- but might break (including causing UB) on different compiler versions or different platforms.

&[T] and &[(T, ())] are not AFAIK guaranteed by the UCG to have the same layout, even though T and (T, ()) are. However it's very unlikely that we would change the order or padding of the pointer/length pair, so they happen to be layout-compatible in practice and will probably remain as such.

@lqd
Copy link
Member

lqd commented Aug 30, 2021

Let's merge this, then.

@lqd lqd merged commit 31e8fa8 into rust-lang:master Aug 30, 2021
@ecstatic-morse ecstatic-morse deleted the rel-join-empty-tuple branch August 30, 2021 20:28
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