-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
perf: improve equality comparison performance #173
perf: improve equality comparison performance #173
Conversation
Thanks for the PR! Are you able to share the benchmarks you used? I would love to run them locally and validate the results. |
Sure, here is the link: https://github.com/Maksimka101/equatable_benchmark |
Hello Any news?🙂 Do I need to fix something or provide more information? |
Apologies for the delay! I was slow to respond due to the holidays but I should have time to review this in the next day or two. Thanks again! |
Thanks. No need to hurry. I forgot that normal people relax on holidays🙂. Happy new year |
Hello Can we merge this? |
Just saw this. besides that I think it would definitely great to get this merged, why not borrow the optimization of |
Will review this later today apologies that it fell through the cracks. Looks like ci was failing but just re-triggered ci to see the logs. |
Looks like some tests are missing. I’ll take a look at the benchmarks and will see if I can get this merged later today. Apologies for the delay! |
@Maksimka101 I've added benchmarks and run them against the implementation on master and am seeing the following results: branch: master
branch: feat/performance-improvement
MacBook Pro (M1 Pro, 16GB RAM) I'm not able to reproduce the significant performance increase you describe. Maybe Let me know if you're still able to reproduce the ~20% performance improvement, thanks! |
Because the classes should be immutable (e.g. all fields should be final and have a const constructor). Classes that use |
The thing is, our benchmarks differ. It's hard to notice, but in mine, all the collections are identical, while in yours, almost all of them differ. That's why mine gives such small values – it goes through all the fields, all the elements of the collection. Whereas in your benchmark, differences are found almost immediately, and the method completes quickly Try to add this benchmark: _runBenchmark(
'CollectionEquatable (large) (all equal)',
(index) => CollectionEquatable(
list: List.generate(100, (i) => 1024),
map: Map.fromEntries(
// ignore: prefer_const_constructors
List.generate(100, (i) => MapEntry('${1024}', 1024)),
),
set: Set.from(List.generate(100, (i) => 1024)),
),
); It'll give you the following results:
In my app, one field in a huge store object was changing, and this happened very frequently. My case is not the most common, but it's also not rare. The comparison operation should be fast not only for different values but also for identical ones :) |
Thanks for the reply! I updated the benchmarks and am able to reproduce the slowdown in larger static datasets. Will look at it a bit more closely in the next few days. I want to take a closer look at why DeepCollectionEquality is suboptimal and ideally open a PR to improve the performance in package collection so that more packages benefit from it. |
I've researched it a bit and I think the DeepCollectionEqality is slow by design due to is flexible API. It creates a map to compare sets and probably maps. Also, it doesn't use the == operator directly so it has some overhead But hope I'm wrong :) |
Planning to land this ASAP but it is still missing quite a few tests for the various equatable_utils APIs (e.g |
lib/src/equatable_utils.dart
Outdated
final unitA = a[i]; | ||
final unitB = b[i]; | ||
if (_isEquatable(unitA) && _isEquatable(unitB)) { | ||
return unitA == unitB; |
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 implementation is incorrect since this will incorrectly return true when the first two objects are equal in a list of props (even if the rest aren't):
class Person with EquatableMixin {
Person({required this.name});
final String name;
@override
List<Object?> get props => [name];
}
test('...', () {
final alice = Person(name: 'Alice');
expect(equals([alice, null], [alice, -1]), isFalse);
});
The above test incorrectly fails.
lib/src/equatable_utils.dart
Outdated
if (_isEquatable(unitA) && _isEquatable(unitB)) { | ||
return unitA == unitB; | ||
} else if (unitA is Set && unitB is Set) { | ||
return _setEquals(unitA, unitB); |
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.
Same goes for the rest of these -- we can't just return true early without actually checking each unit in the list.
You are right. I found one bug in the Also, I've added the Benchmark details (when compiled to the exe file)With the inline pragma
Without the inline pragma
|
bool iterableEquals(Iterable<Object?> a, Iterable<Object?> b) { | ||
assert( |
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.
Thoughts on calling setEquals
instead of asserting here?
if (a is Set && b is Set) return setEquals(a, b);
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 expect anyone to use this function. I'm not sure it's even exported. So I'd prefer to leave it simple, without extra ifs
@Maksimka101 with the latest changes, the benchmarks indicate the performance is worse than the original (in which we used |
You're right, but only partially. In JIT mode, the current version is indeed faster in 5 out of 8 tests. But in AOT mode, the new implementation is significantly faster in 7 out of 8 tests. And this is very important because most Dart programs are compiled AOT I'll try to figure out why it performs so poorly in JIT, and I'll also look at the difference when compiling to JS Edit: New implementation is significantly faster in 6 of 8 tests and slightly slower in 2 of 8 tests when compiled to JS. (compiled with the -O2 flag, launched on node v22.4.0) |
@felangel I improved performance in almost every test across all build options. The largest performance drop is 6.3% in JS for the PrimitiveEquatable benchmark. The biggest performance boost is also in JS, in CollectionEquatable (static, medium), with a 356% increase :) In the end, I just marked 2 functions for inline, but I ran so many experiments... By the way, you can check out the performance comparison charts here: https://docs.google.com/spreadsheets/d/1e5g_URJ6oFc76e-YYhDXVBeqqzV7ZN0vDuh-3nPxGGY/edit?usp=sharing |
Thanks! I’ll take a closer look later today 💙 |
Will update the benchmarks and include both AOT and JIT versions and plan to merge and publish this later today. Sorry for the delay and thanks for all your contributions and time 💙 |
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.
LGTM thanks so much for the energy and time you put into this -- I really appreciate it! 💙
Status
READY
Breaking Changes
NO
Description
I've noticed that the performance of comparing objects with collections can be significantly enhanced. These optimizations in my application led to a considerable increase in FPS, approximately by 3 times. Additionally, I've conducted tests to compare the performance between the old and new approaches
Todos
Impact to Remaining Code Base
This PR will affect: