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

Add back base Union/Record classes #2194

Merged
merged 2 commits into from
Oct 5, 2020
Merged

Conversation

alfonsogarciacaro
Copy link
Member

When trying to compile Fable.SimpleJson with Nagareyama we realized the code that relied on Unions and Record inheriting some behavior (like toJSON implementations) could easily break so I've tried to apply @chrisvanderpennen solution (see #2160) to bring back the base classes for unions and records ...although it seems @Zaid-Ajaj already managed to fix Fable.SimpleJson so maybe this is already outdate 😅

Zaid-Ajaj/Fable.SimpleJson#54

In microbenchmarks initializing derived classes takes longer, but I couldn't see an appreciable difference when running the benchmark (see #2186) between this branch and current nagareyama.

@ncave
Copy link
Collaborator

ncave commented Oct 5, 2020

@alfonsogarciacaro The other option is to store the union/record base type in Reflection, so it can be properly identified if needed at runtime, instead of looking at the object shape. Using Reflection understandably has a perf cost if (over)used by default for type checking (unless we can optimize it more). If using base classes is faster than using Reflection and/or works better and doesn't impact performance, then sure, whatever makes sense.

@ncave
Copy link
Collaborator

ncave commented Oct 5, 2020

@alfonsogarciacaro I can confirm there is no negative perf impact on the FCS benchmark from this PR.
Actually there is a minor perf gain (~ 3% faster, which is small but still an improvement nonetheless).

@alfonsogarciacaro
Copy link
Member Author

Ah, great. Thanks for checking that! Maybe the perf gain comes because we can call now Equals/CompareTo directly for unions and records instead of checking the shape of the object every time. In a separate PR we could try to check if generating a specialized Equals/CompareTo method (like the F# compiler does) for unions/record/tuples used as Map/dictionary keys improves things 👍

@alfonsogarciacaro alfonsogarciacaro merged commit fd90f93 into nagareyama Oct 5, 2020
@alfonsogarciacaro alfonsogarciacaro deleted the union-record-base branch October 5, 2020 21:20
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.

2 participants