Skip to content

feat!: Introduce new PriorityOrderedSet #57

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

Merged
merged 5 commits into from
Apr 9, 2025
Merged

feat!: Introduce new PriorityOrderedSet #57

merged 5 commits into from
Apr 9, 2025

Conversation

luanpotter
Copy link
Member

@luanpotter luanpotter commented Apr 7, 2025

This introduces a new type of ordered set that takes a mapper function instead of a comparing function, and thus is able to perform some optimizations much needed in Flame.

In order not to break backwards compatibility too much, the following approach is taken:

  • OrderedSet becomes an "interface" (in Dart world that is an abstract class)
  • We provide two implementations, ComparingOrderedSet is the old one, new one is PriorityOrderedSet
  • You can use static methods on the OrderedSet interface to create any sets with helper functions
  • QueryableOrderedSet takes in the underlying OrderedSet so it works with both versions

The new PriorityOrderedSet uses a splay tree map instead of splay tree set and thus caches the priorities on the map keys. That means you can safely update priorities or have dynamic priority functions and have consistent behaviour before rebalancing.

This is a much nicer underlying fix for flame-engine/flame#3363 than how we did it on the Flame side.

Copy link

github-actions bot commented Apr 7, 2025

Benchmark Results

Package ordered_set:

Benchmarks Current Branch
[luan.priority-os]
Base Branch
[main]
Diff
Iteration Benchmark - Comparing 241.286 μs [-] [-]
Insertion and Removal Benchmark - Comparing 360.971 μs [-] [-]
Comprehensive Benchmark - Comparing 15338.153 μs [-] [-]
Iteration Benchmark - Priority 237.318 μs [-] [-]
Insertion and Removal Benchmark - Priority 282.170 μs [-] [-]
Comprehensive Benchmark - Priority 14558.543 μs [-] [-]
Iteration Benchmark [-] 242.255 μs [-]
Insertion and Removal Benchmark [-] 326.397 μs [-]
Comprehensive Benchmark [-] 15305.254 μs [-]

Benchmarks provided with 💙 by Dart Benchmark Action.

@luanpotter luanpotter changed the title feat!: Add new PriorityOrderedSet [WIP] feat!: Introduce new PriorityOrderedSet Apr 7, 2025
@luanpotter luanpotter marked this pull request as ready for review April 7, 2025 15:42
@luanpotter luanpotter requested a review from spydon April 7, 2025 15:42
@coveralls
Copy link

coveralls commented Apr 7, 2025

Pull Request Test Coverage Report for Build 14359965723

Details

  • 136 of 145 (93.79%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-2.8%) to 94.581%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/ordered_set.dart 24 25 96.0%
lib/comparing_ordered_set.dart 46 48 95.83%
lib/queryable_ordered_set.dart 15 21 71.43%
Files with Coverage Reduction New Missed Lines %
lib/ordered_set.dart 1 95.92%
Totals Coverage Status
Change from base Build 14295806377: -2.8%
Covered Lines: 192
Relevant Lines: 203

💛 - Coveralls

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

I'm not too fond of comparing as a name, maybe MappedOrderedSet or something instead?

@luanpotter
Copy link
Member Author

I also don't like the names, but MappedOrderedSet is essentially the other one (takes a mapping function)! the og one takes a comparator function instead

@spydon
Copy link
Member

spydon commented Apr 8, 2025

I also don't like the names, but MappedOrderedSet is essentially the other one (takes a mapping function)! the og one takes a comparator function instead

Ah right, of course, maybe the other one should be called MappedOrderedSet then since priority is an implementation detail for Flame?
For Comparing, it feels like it shouldn't be a verb/noun there, but an adjective?

@luanpotter
Copy link
Member Author

Yeah I agree with Mapping for the other one. One of the helper functions is already called that. We migh need to rework those names, which is fine.
For the OG, Comparing is an adjective no? The same way that Mapping is an adjective form of the verb map.

@luanpotter
Copy link
Member Author

But I am open to other names, maybe ComparatorBased would be more clear but it's clunky.

@spydon
Copy link
Member

spydon commented Apr 8, 2025

But I am open to other names, maybe ComparatorBased would be more clear but it's clunky.

I like that, go for it!

Oooor could we have OrderedSet not being abstract (and have that being the comparator based one) and MappedOrderedSet just extending it and overriding necessary parts? Or the other way around might be easier? 🤔

@luanpotter
Copy link
Member Author

MappedOrderedSet is a completely different implementation, so what you could have is they both extend a BaseOS and then OG can be called OS - but I think the way I did is better because essentially we are encouraging people to only use the OrderedSet interface + the static methods to create them. People shouldn't even be looking at the specific implementations:

  OrderedSet set = OrderedSet.comparing/mapping/etc(parameters);
  // using the interface allows them to switch the impl at any time

@luanpotter
Copy link
Member Author

I'm happy with ComparatorBase, will update later today!

@spydon
Copy link
Member

spydon commented Apr 8, 2025

MappedOrderedSet is a completely different implementation, so what you could have is they both extend a BaseOS and then OG can be called OS - but I think the way I did is better because essentially we are encouraging people to only use the OrderedSet interface + the static methods to create them. People shouldn't even be looking at the specific implementations:

  OrderedSet set = OrderedSet.comparing/mapping/etc(parameters);
  // using the interface allows them to switch the impl at any time

Alright, sounds good! I guess we should update some docs in the readme too.

@luanpotter
Copy link
Member Author

@spydon I spent some time thinking about and after much deliberation I reached these conclusions:

  • the best names I think are ComparingOrderedSet and MappingOrderedSet, making it clear what each is based off and both consistently in template. I updated all references and names accordingly
  • you cannot manually construct a LinkedHashSet in dart, as it is a lint issue. The literals are preferred, and when using the literals we have to use Set, or force a cast. I opted for leaving as is (we can always revist)
  • I updated and revitalized the README file

@luanpotter luanpotter requested a review from spydon April 9, 2025 01:05
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! 😍

@spydon spydon merged commit d89d9f6 into main Apr 9, 2025
6 checks passed
@spydon spydon deleted the luan.priority-os branch April 9, 2025 15:00
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.

3 participants