-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[pigeon] Optimize data class equality and hashing in Dart, Kotlin, java, and Swift, adds equality in other languages #11140
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
Open
tarrinneal
wants to merge
43
commits into
flutter:main
Choose a base branch
from
tarrinneal:effic
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+10,792
−1,635
Open
Changes from all commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
82065a8
Improve efficiency in some methods
tarrinneal 9861c00
analyze this
tarrinneal 11daedd
format
tarrinneal e197fc0
less repeating, and less listing
tarrinneal cbf4096
allow individual class types to not have colliding hashes
tarrinneal 935aa47
improve deep equals for non-collections
tarrinneal 4e9541f
allowing for NaN or other cases where identical is better
tarrinneal f30b44c
more tests, add other languages, unify behavior across platforms
tarrinneal 08a79f3
first pass at gobject
tarrinneal 8cb51ac
Merge branch 'main' of https://github.com/flutter/packages into effic
tarrinneal ddc41c2
analyze
tarrinneal 4ad3636
delete random extra files
tarrinneal 0a08ef5
fix kotlin generator change that broke unit test
tarrinneal 5855b23
fix broken gobject and cpp code
tarrinneal f99e64a
more bugs on languages I can't compile locally
tarrinneal 101fb0f
last few issues I hope
tarrinneal 7309d17
not being able to run things locally sucks
tarrinneal 24d0f23
bigobj
tarrinneal 21ad393
consolidate tests
tarrinneal 94a58c8
revert project files
tarrinneal 3fdd987
More consistency across languages
tarrinneal b920f50
revert -0.0 changes
tarrinneal 7df481e
gen example
tarrinneal 25021e7
Merge branch 'main' of https://github.com/flutter/packages into effic
tarrinneal 4cbfdcd
memecmp
tarrinneal 8c75a36
re-normalize -0.0 in hash methods to create consistent behavior acros…
tarrinneal 655229c
finish tests
tarrinneal 80d8bbc
lints and reverting of accidentally pushed files
tarrinneal a57dd6f
hopeful test fix
tarrinneal 4ded545
delete file that shouldn't ever have existed
tarrinneal 8698cb5
more tests and windows method rework
tarrinneal 7355a96
bug fixes and more robust map==
tarrinneal 45d8207
fix linix
tarrinneal 97b8e8c
one last linux fix
tarrinneal 38cffb2
nits and nats
tarrinneal a2a80b5
dentist has been busy today
tarrinneal b5b4506
Merge branch 'main' of https://github.com/flutter/packages into effic
tarrinneal 1305a1d
fix std::map hash
tarrinneal a5ce9cc
even more better
tarrinneal 6d92541
Merge branch 'main' of https://github.com/flutter/packages into effic
tarrinneal eba918b
revert dumb change
tarrinneal 5097bdc
namespace
tarrinneal 0a5adee
Triple checked methods
tarrinneal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Capturing for posterity, this has the same inefficiency as the previous iteration, where we are feeding known-type values through a big type-inspection chain instead of dispatching to specific-type equality checkers.
Not something we need to address now since we still mostly expect these to be used in tests where efficiency doesn't matter, but something to keep in mind later if it performance ever comes up.
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.
This doesn't actually, any non-collections well return first before any type checking. Even collections are only checking for collection types.
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.
You're probably thinking about the codec, which I intend to change so known types bypass the type checking soon(tm)
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.
I'm not following. The first line in this statement is calling
pigeonDeepEqualswith two strings. We know at compile time they are strings. But we are callingpigeonDeepEqualsfor them which will do runtime checks for whether they are:ListsMapsDoublesFloatsand only then, after all of that, check that the strings are equal.
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.
the first thing pigeon deep equals does is return if identical
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.
Right, but that's only pointer equality for most types. For two instances of
"Foo",a == bat the top isfalse, onlya.equals(b)at the very end istrue, after all the type checks. Similarly, twoMapinstance variables that aren't actually the same map, but will evaluate to the same under the deep equality evaluation, will go through 5 pointless type checks first. Similarly withList. Similarly withDoubleandFloat.Basically, in any case where we actually need all this logic in the first place and can't just rely on
==alone, we are doing a bunch of type checks.Uh oh!
There was an error while loading. Please reload this page.
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.
That is a detail I missed. I've resolved this in every language (that I can).
My future work for changing the codec calls to skip the type checking (this will be relevant once native interop lands) will change this to avoid type checking also.
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.
I don't think the quick fix of moving the non-trivial equality check to the top is something we want to do, because it may be non-trivially expensive, and duplicative of specific checks. E.g., I'm almost positive this will compare non-equal strings twice in Obj-C instead of once, which is potentially more expensive than the thing I was describing in the initial comment.
The fix here (which again, doesn't need to be done now, I just want to capture it for later reference) isn't to change how
pigeonDeepEqualsworks, it's to not generate calls topigeonDeepEqualsin the first place when we already know the type. Instead, all the specific checks should be helpers (pigeonMapDeepEquals, etc.),pigeonDeepEqualsshould be implemented using those helpers, and the generator for class equality should be calling the correct helper for each field instead of the generic helper.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.
That is true, reverted.