Replace pseudo-extensible Foreign with a big union#6125
Merged
Conversation
This replaces the existential-based `Foreign` type with an exhaustive enumeration of the wrapped types. The original intention was that the wrapped types would be extensible at runtime, but I'm not sure that's really possible, and in any case we haven't done it. Doing this requires moving the type into the `Stack` module, because some cases recursively refer to `Val`. So the `U.R.Foreign` module is gone. The namespace still exists for other related stuff. The intention of this change was to improve equality/ordering of wrapped values. Before, we had to do repeated testing of `Reference` values to determine what the wrapped types even were. This is expensive, and the expense is worse if you're low on the list. With the union, figuring out the type is a case analysis, and is much faster. However, this also has other positive effects. Many uses of `unsafeCoerce` have been eliminated, and I believe I even found an unnoticed type error in one of the AVRO cases. Some compiler panics should get slightly better information, too, or a more useful panic will happen in place of a segfault.
Member
|
@dolio CI failed with some weeder issue, otherwise looks good, you can merge when ready. |
Contributor
Author
|
Another @ceedubs sanity check would be appreciated. One thing I encountered was that the pointer equality stopped working for one of the tls things in the transcripts (thought not everything). But I was actually able to replace it with normal equality. I tried to switch to doing as little pointer equality as possible. It'd be good to know if everything seems covered for cloud. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 replaces the existential-based
Foreigntype with an exhaustive enumeration of the wrapped types. The original intention was that the wrapped types would be extensible at runtime, but I'm not sure that's really possible, and in any case we haven't done it.Doing this requires moving the type into the
Stackmodule, because some cases recursively refer toVal. So theU.R.Foreignmodule is gone. The namespace still exists for other related stuff.The intention of this change was to improve equality/ordering of wrapped values. Before, we had to do repeated testing of
Referencevalues to determine what the wrapped types even were. This is expensive, and the expense is worse if you're low on the list. With the union, figuring out the type is a case analysis, and is much faster.However, this also has other positive effects. Many uses of
unsafeCoercehave been eliminated, and I believe I even found an unnoticed type error in one of the AVRO cases. Some compiler panics should get slightly better information, too, or a more useful panic will happen in place of a segfault. I was also able to easily improve the universal equality by making more wrapped values use(==)instead of pointer equality, since every case is now listed.Base tests pass.
Benchmarks look mostly the same, except:
I assume that's looking up
Byteskeys or something.I was also running a benchmark locally that did comparisons in a loop, testing 16-byte
Bytesvalues vs. twoNatvalues. Before these changes, theBytesversion was ~50% slower, and now they're approximately the same. There is an actual difference (Bytesbeing slower) if you look hard enough, but they're close enough to get lost in the noise unless you're doing almost exclusively comparisons.