-
Notifications
You must be signed in to change notification settings - Fork 292
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
[Python] Typeinfo object identity equality #2735
[Python] Typeinfo object identity equality #2735
Conversation
Looks good to me! What do you think @alfonsogarciacaro? Could you have a look at this? Do we btw have this problem in JavaScript? |
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, thanks for fixing!
@dbrattli Sorry, I didn't realize you mentioned me in the thread 😅 I wish GH told you when a notification is because of a direct mention (maybe it does and I'm missing something?).
In the case of types, it does seems that .NET actually produces the same instance, even when using let t1 = typeof<int list>
let t2 = typedefof<obj list>.MakeGeneric([|typeof<int>|])
obj.ReferenceEquals(t1, t2) // true I guess a cache as in this PR would have the same effect, although I'm always reluctant to include caches in library code unless it's explicit because they may grow out of control. Another solution could be to have a special implementation of ReferenceEquals for built-in types (in the case of |
@dbrattli Sorry, I've been thinking about this, and I'm still unsure it's a good idea to include a cache in the library. Particularly not just to try to make @thautwarm Is there a reason you cannot use |
I think your idea of avoiding cache makes sense, but a function that creates typeinfo should be pure, I think it is okay to cache such a function.
I can always use Concretely, type-driven data parsing/validation via F# reflection needs to map type objects (statically resolved in F# hence more reliable) to handler functions, but map lookup using structural hashing and structural equality is expensive. As can be seen in the following example code, serializing/deserializing nested data structure will compare types frequently, while using object identity for hashing and equality makes such recursion really cheap. let rec deserialize (registeredHandlers: Map<System.Type, Handler>) (t: System.Type) (stream): obj =
if t is record/union then // composite types
// also lookup the map and call 'deserialize' recursively
else
match Map.tryFind t registeredHandlers with
| None -> ..
| Some handler -> handler.deserialize stream
|
I did some rudimentary benchmark on JSON parsing, which shows my type-driven data parsing is slow due to F# reflection. I haven't make tests against this PR to show how much the performance gets improved, as it's difficult to locally use |
Thanks for your reply @thautwarm! Some notes:
Then you can edit |
Thanks for the good explanation @alfonsogarciacaro. I agree that caches (especially unbounded) are scary (and if the user do not know about them). So we need to figure out a better way to solve this or revert. |
This addresses fable-compiler/Fable.Python#42 in another approach. It does not need a change to the compiler code.
Such approach leverages a theory(need confirmation?) that .NET type identities are decided by the fullname and its generic type arguments. The only exception is anonymous records, which use unordered field info for equality.
This PR implements type comparisons that consume constant space and constant time.