-
Notifications
You must be signed in to change notification settings - Fork 99
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
Serokell: [Milestone-2] Optimize PersistenOrderedMap.mo #664
base: master
Are you sure you want to change the base?
Conversation
Some results of our experiments we believe show where the Motoko compiler has the potential to perform better optimizations:
|
No changes in logic so far, just simple refactoring
Add `MapOps` class with the following signature: public class MapOps<K>(compare : (K,K) -> O.Order) { public func put<V>(rbMap : Map<K, V>, key : K, value : V) : Map<K, V> public func fromIter<V>(i : I.Iter<(K,V)>) : Map<K, V> public func replace<V>(rbMap : Map<K, V>, key : K, value : V) : (Map<K,V>, ?V) public func mapFilter<V1, V2>(f : (K, V1) -> ?V2, rbMap : Map<K, V1>) : Map<K, V2> public func get<V>(key : K, rbMap : Map<K, V>) : ?V public func delete<V>(rbMap : Map<K, V>, key : K) : Map<K, V> public func remove< V>(rbMap : Map<K, V>, key : K) : (Map<K,V>, ?V) }; The other functionality provided as standalone functions, as they don't require comparator: public type Direction = { #fwd; #bwd }; public func iter<K, V>(rbMap : Map<K, V>, direction : Direction) : I.Iter<(K, V)> public func entries<K, V>(m : Map<K, V>) : I.Iter<(K, V)> public func keys<K, V>(m : Map<K, V>, direction : Direction) : I.Iter<K> public func vals<K, V>(m : Map<K, V>, direction : Direction) : I.Iter<V> public func map<K, V1, V2>(f : (K, V1) -> V2, rbMap : Map<K, V1>) : Map<K, V2> public func size<K, V>(t : Map<K, V>) : Nat public func foldLeft<Key, Value, Accum>( combine : (Key, Value, Accum) -> Accum, base : Accum, rbMap : Map<Key, Value> ) : Accum And foldRight with the same signature as foldLeft The following functions are new for the API: - MapOps.put, MapOps.delete - MapOps.fromIter, entries, keys, vals - MapOps.mapFilter, map - foldLeft, foldRight
Problem: now order is not consistent within new module and with old modules as well. Solution: make the map argument always go first
In addition to tests this patch removes `direction` argument from `keys` and `values` function to keep them simple and provides a new function `Map.empty` to create a map without knowing its internal representation.
* rename `rbMap` into `m` in signature for brevity & consistent language * rename `rbMap` into `map` in examples for brevity & encapsulation sake * rename `tree` into `map` in doc comments for the encapsulation sake
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 looks very nice. Added some comments inline.
Compared with https://ocaml.org/manual/5.2/api/Map.S.html, do you think we need to add any more operations now or should we leave them to later? There are some basic ones like OCaml.mem (Motoko has
) to check a key exists, and OCaml for_all/exists
(Motoko all/exists
) for computing predicates (presumeably short-circuiting).
src/PersistentOrderedMap.mo
Outdated
// TODO: a faster, more compact and less indirect representation would be: | ||
// type Map<K, V> = { | ||
// #red : (Map<K, V>, K, V, Map<K, V>); | ||
// #black : (Map<K, V>, K, V, Map<K, V>); | ||
// #leaf | ||
//}; | ||
// (this inlines the colors into the variant, flattens a tuple, and removes a (now) redundant option, for considerable heap savings.) |
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 guess we can delete the TODO now (all addressed right?)
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.
fixed
src/PersistentOrderedMap.mo
Outdated
/// | ||
/// `MapOps` contains methods that require `compare` internally: | ||
/// operations that may reshape a `Map` or should find something. | ||
public class MapOps<K>(compare : (K,K) -> O.Order) { |
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.
public class MapOps<K>(compare : (K,K) -> O.Order) { | |
public class MapOps<K>(compare : (K, K) -> O.Order) { |
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.
fixed
src/PersistentOrderedMap.mo
Outdated
// #leaf | ||
//}; | ||
// (this inlines the colors into the variant, flattens a tuple, and removes a (now) redundant option, for considerable heap savings.) | ||
// It would also make sense to maintain the size in a separate root for 0(1) access. |
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.
// It would also make sense to maintain the size in a separate root for 0(1) access. | |
// It would also make sense to maintain the size in a separate root for 0(1) access. |
What do you think about reconsidering this now? I think @luc was quite keen on adding this and I think it would be useful for, e.g. coping a tree to an array etc. Does it complicate the operations much?
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.
Yes, we will make it. We hope it will not diminish performance much, but just in case we will check the benchmark again.
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.
Agreed to make it in the 3rd Milestone (#662)
/// Cost of empty map creation | ||
/// Runtime: `O(1)`. | ||
/// Space: `O(1)` | ||
public func empty<V>() : Map<K, V> = #leaf; |
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 made we wonder is we should rename #leaf to #empty, so users can just use #empty for the empty map, and also to distinguish this from the old RBTree #leaf constructor. Whaddya think?
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 think it makes sense to have it for the encapsulation. For example, when we add the size field the empty
method will be necessary.
src/PersistentOrderedMap.mo
Outdated
/// import Debug "mo:base/Debug"; | ||
/// | ||
/// let mapOps = Map.MapOps<Nat>(Nat.compare); | ||
/// let rbMap = mapOps.fromIter<Text>(Iter.fromArray([(0, "Zero"), (2, "Two"), (1, "One")])); |
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.
Did you decide not to rename rbMap
to map
throughout, or is that for a separate PR?
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.
Yeah, we just did not rebase this branch yet because I was considering that there are probably some open discussions in the #654
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.
rebased
/// where `n` denotes the number of key-value entries stored in the map. | ||
/// | ||
/// Note: Full map iteration creates `O(n)` temporary objects that will be collected as garbage. | ||
public func entries<V>(m : Map<K, V>) : I.Iter<(K, V)> = iter(m, #fwd); |
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.
RBTree.mo had methods rb.entries, rb.entriesRev
and static function RBTree.iter(rb, dir)
. Should we just consolidate on either entries()
and entriesRev()
or iter(rb, dir)
?
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.
Related: If we keep iter(rb, dir)
, can we move type Direction
into MapOps
?
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.
Should we just consolidate on either entries() and entriesRev() or iter(rb, dir)?
We don't have a strong opinion about this. We can do either way or just add entriesRev()
.
We was reading design.md, which says that entries
should be, but it's clarifying precisely this case.
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.
@crusso Should we raise this question in the Slack?
src/PersistentOrderedMap.mo
Outdated
(#red (l, x, y, r)) | ||
}; | ||
case _ { | ||
Debug.trap "RBTree.red" |
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.
Debug.trap "RBTree.red" | |
Debug.trap "PersistentOrderedMap.red" |
Are there others that need fixing?
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.
Fixed, I haven't found more "RBTree" stuff in the code.
This PR didn't touch folds and map as benchmark showed us that it performs worse.
This is an MR for the 3rd Milestone of the Serokell's grant about improving Motoko's base library. The main goal of the PR is to introduce a new functional implementation of the set data structure to the' base' library. Also, it brings a few changes to the new functional map that was added in #664 , #654 . # General changes: * rename `PersistentOrderedMap` to `OrderedMap` (same for the `OrderedSet`) * improve docs # Functional Map changes: ## New functionality: + add `any`/`all` functions + add `contains` function + add `minEntry`/`maxEntry` ## Optimizations: + Store `size` in the Map, [benchmark results](serokell#35) ## Fixup: + add `entriesRev()`, remove `iter()` # NEW functional Set: The new data structure implements an ordered set interface using Red-Black trees as well as the new functional map from the 1-2 Milestones. ## API implemented: * Basic operations (based on the map): `put`, `delete`, `contains`, `fromIter`, etc * Maps and folds: `map`, `mapFilter`, `foldLeft`, `foldRight` * Set operations: `union` , `intersect`, `diff`, `isSubset`, `equal` * Additional operations (as for the `OrderedMap`): `min`/`max`, `all`/`some` ## Maintainance support: * Unit, property tests * Documentation ## Applied optimizations: * Same optimizations that were useful for the functional map: * inline node color * float-out exceeded matching in iteration * `map`/`filterMap` through `foldLeft` * direct recursion in `foldLeft` * [Benchmark results for all four optimizations together](serokell#27) * store size in the root of the tree, [benchmark results](serokell#36 (comment)) * Pattern matching order optimization, [benchmark results](serokell#36 (comment)) * Other optimizations: * Inline code of `OrderedMap` instead of sharing it, [benchmark results](serokell#25) * `intersect` optimization: use order of output values to build the resulting tree faster, see serokell#39 * `isSubset`, `equal` optimization: use early exit and use order of subtrees to reduce intermediate tree height, see serokell#37 ## Rejected optimizations: * Nipkow's implementation of set operation [Tobias Nipkow's "Functional Data Structures and Algorithms", 117]. Initially, we were planning to use an implementation of set operations (`intersect`, `union`, `diff`) from Nipkow's book. However, the experiment shows that naive implementation with a simple size heuristic performs better. [The benchmark results](serokell#33) are comparing 3 versions: * persistentset_baseline -- original implementation that uses Nipkow's algorithms. However, the black height is calculated before each set operation (the book assumes it's stored). * persistentset_bh -- the same as the baseline but the black height is stored in each node. * persistentset -- naive implementation that looks up in a smaller set and modifies a bigger one (it gives us `O(min(n,m)log((max(n,m))` which is very close to Nipkow's version). Sizes of sets are also stored but only in the root. The last one outperforms others and keeps a tree slim in terms of byte size. Thus, we have picked it. ## Final benchmark results: ### Collection benchmarks | |binary_size|generate|max mem|batch_get 50|batch_put 50|batch_remove 50|upgrade| |--:|--:|--:|--:|--:|--:|--:|--:| |orderedset+100|218_168|186_441|37_916|53_044|121_237|127_460|346_108| |trieset+100|211_245|574_022|47_652|131_218|288_429|268_499|729_696| |orderedset+1000|218_168|2_561_296|520_364|69_883|158_349|170_418|3_186_579| |trieset+1000|211_245|7_374_045|633_440|162_806|383_594|375_264|9_178_466| |orderedset+10000|218_168|40_015_301|320_532|84_660|192_931|215_592|31_522_120| |trieset+10000|211_245|105_695_670|682_792|192_931|457_923|462_594|129_453_045| |orderedset+100000|218_168|476_278_087|3_200_532|98_553|230_123|258_372|409_032_232| |trieset+100000|211_245|1_234_038_235|6_826_516|222_247|560_440|549_813|1_525_692_388| |orderedset+1000000|218_168|5_514_198_432|32_000_532|115_836|268_236|306_896|4_090_302_778| |trieset+1000000|211_245|13_990_048_548|68_228_312|252_211|650_405|642_099|17_455_845_492| ### set API | |size|intersect|union|diff|equals|isSubset| |--:|--:|--:|--:|--:|--:|--:| |orderedset+100|100|146_264|157_544|215_871|28_117|27_726| |trieset+100|100|352_496|411_306|350_935|201_896|201_456| |orderedset+1000|1000|162_428|194_198|286_747|242_329|241_938| |trieset+1000|1000|731_650|1_079_906|912_629|2_589_090|4_023_673| |orderedset+10000|10000|177_080|231_070|345_529|2_383_587|2_383_591| |trieset+10000|10000|3_986_854|21_412_306|5_984_106|46_174_710|31_885_381| |orderedset+100000|100000|190_727|267_008|402_081|91_300_348|91_300_393| |trieset+100000|100000|178_863_894|209_889_623|199_028_396|521_399_350|521_399_346| |orderedset+1000000|1000000|205_022|304_937|464_859|912_901_595|912_901_558| |trieset+1000000|1000000|1_782_977_198|2_092_850_787|1_984_818_266|5_813_335_155|5_813_335_151| ### new set API | |size|foldLeft|foldRight|mapfilter|map| |--:|--:|--:|--:|--:|--:| |orderedset|100|16_487|16_463|88_028|224_597| |orderedset|1000|133_685|131_953|1_526_510|4_035_782| |orderedset|10000|1_305_120|1_287_495|28_455_361|51_527_733| |orderedset|100000|13_041_665|12_849_418|344_132_505|630_692_463| |orderedset|1000000|130_428_573|803_454_777|4_019_592_041|7_453_944_902| --------- Co-authored-by: Andrei Borzenkov <andreyborzenkov2002@gmail.com> Co-authored-by: Andrei Borzenkov <root@sandwitch.dev> Co-authored-by: Sergey Gulin <sergeygulin96@gmail.com> Co-authored-by: Claudio Russo <claudio@dfinity.org>
This is an MR for the 2nd Milestone of Serokell's grant work aimed to improve Motoko's base library.
Within the milestone, we made performance experiments over the new module
PersistentOrderedMap.mo
and tried to optimize it. This PR contains the optimization that exhibited good:iter
: move matching on the direction out: 7-15% speed up, benchmark resultsfoldLeft
/foldRight
: use direct recursion: ~80% speed up, benchmark resultsmapFilter
: usefoldLeft
instead ofiter
: 20-40% speed up, benchmark resultsget
and folds which become ~7% slower, benchmark resultsAlso, it makesInternal
module public since static calls perform better, benchmark resultsUPDATE:
After some discussions, we settled on the following additional changes:
MapOps
for user convenience. As a side effect, operations that were not in the MapOps got slower on ~50 instructions per call (~2500 per 50-batch) on the benchmarks.Internal
module private againSee benchmark results of the whole update.
This MR is following up #654
Final performance comparison
Map comparison
Initial results
| |binary_size|generate|max mem|batch_get 50|batch_put 50|batch_remove 50|upgrade| |--:|--:|--:|--:|--:|--:|--:|--:| |persistentmap_100|187_090|201_602|42_600|51_044|122_234|124_817|440_282| |persistentmap_baseline_100|191_689|226_832|45_672|49_945|139_070|134_191|512_457| |rbtree_100|189_877|225_155|42_540|50_045|135_367|133_686|565_657| |persistentmap_1000|187_090|2_724_937|568_248|68_227|160_005|168_031|4_153_206| |persistentmap_baseline_1000|191_689|3_133_922|612_880|67_416|184_117|181_732|4_878_488| |rbtree_1000|189_877|3_118_490|580_948|67_516|181_375|180_396|5_409_805| |persistentmap_10000|187_090|45_412_473|480_360|84_528|195_152|214_853|41_210_098| |persistentmap_baseline_10000|191_689|51_438_500|520_360|83_365|227_294|231_116|48_561_107| |rbtree_10000|189_877|51_301_049|520_428|83_465|224_135|230_257|53_866_589| |persistentmap_100000|187_090|531_616_890|4_800_360|98_864|233_003|258_058|542_245_665| |persistentmap_baseline_100000|191_689|608_157_242|5_200_360|97_912|273_597|277_661|645_876_276| |rbtree_100000|189_877|606_914_881|5_200_428|98_012|270_465|276_802|698_928_540| |persistentmap_1000000|187_090|6_080_971_407|48_000_396|117_446|271_877|307_984|5_422_489_439| |persistentmap_baseline_1000000|191_689|7_005_190_168|52_000_396|116_317|320_359|331_196|6_458_379_331| |rbtree_1000000|189_877|6_993_676_959|52_000_464|116_417|317_299|330_296|6_988_883_198|Persistent map API
Initial results
| |size|foldLeft|foldRight|mapfilter|map| |--:|--:|--:|--:|--:|--:| |persistentmap|100|19_787|20_719|89_105|29_538| |persistentmap_baseline|100|92_138|93_745|169_663|29_048| |persistentmap|1000|167_597|176_129|1_549_566|263_679| |persistentmap_baseline|1000|888_169|899_681|3_556_717|257_766| |persistentmap|10000|1_648_003|1_729_751|32_416_330|2_600_540| |persistentmap_baseline|10000|19_529_035|19_640_763|43_314_564|2_544_359| |persistentmap|100000|16_454_053|17_259_673|384_771_808|129_765_701| |persistentmap_baseline|100000|195_212_923|196_318_318|505_815_170|132_210_500| |persistentmap|1000000|164_559_185|172_575_493|4_435_968_960|1_297_599_225| |persistentmap_baseline|1000000|1_952_082_879|1_963_098_142|5_763_869_513|1_322_035_748|