-
Notifications
You must be signed in to change notification settings - Fork 184
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
Introduce Time/Diff GATs #502
Introduce Time/Diff GATs #502
Conversation
23416e5
to
92c8ca8
Compare
The current state of the PR, to my understanding is:
I think the main remaining question is how much |
e1d3f20
to
0678b80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
The PR slowly introduces GATs for
Time
andDiff
. It largely reproduces work of @antiguru, but in an incremental fashion that helps me at least understand what is going on. The incremental steps are:Layout
containers to provide separate storage for times and diffs, which allows those containers to speak clearly about associated types for those containers. The GATs are immediately converted to owned data, so the transient allocations are still high (increased, even), but the storage opportunities are increased by allowing the containers to store their data in a way that doesn't need to present as&Time
and&Diff
.DiffGat<'_>
andTimeGat<'_>
respectively, which are exactly the GATs offered by the underlying containers, but surfaced upwards through the APIs provided by cursors. Concretely this means thatmap_times()
communicates GATs rather than references, and users of the function can avoid allocation if they do not require it. These commits did not try especially hard to prevent immediately callinginto_owned()
and re-invoking transient allocations.into_owned()
, which are the moments we convert from GATs to owned types, and where we might perform needless allocation. The commit generalizesSemigroup
to act on references, and e.g. the constraint thatTr::Diff : for<'a> Semigroup<Tr::DiffGat<'a>>
to ensure that we can accumulate the GAT types into the owned types directly, without needing to first take ownership. Similar changes for times, though mostly around streamlining the idiom oftime.join(meet)
for a GATtime
(take ownership, thenjoin_assign
).There is work to continue to progressively nibble away at the reliance on the owned types, though unlike keys and values they are unlike to go away: both time and diff need to be mutated (advancing to frontiers and consolidation, respectively). This seems likely to force some owned representation, as one cannot simply rely on the existing read-only data in the backing storage.