Serokell: [Milestone-2] Optimize PersistenOrderedMap.mo #664

GoPavel · 2024-10-14T16:33:30Z

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 results
foldLeft/foldRight: use direct recursion: ~80% speed up, benchmark results
mapFilter: use foldLeft instead of iter: 20-40% speed up, benchmark results
inline node color into the constructor: ~8% less max memory, ~8% speed up on all functions except get and folds which become ~7% slower, benchmark results
optimization of pattern matching order: ~3-15% speed up for basic functions, benchmark results

~~Also, it makes Internal module public since static calls perform better, benchmark results~~

UPDATE:
After some discussions, we settled on the following additional changes:

Optimization: Remove the tuple from the tree definition: 20-25% less memory (max and live heap), speed +-3% depends on the operation, benchmark results
Move all operations into 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.
Make the Internal module private again

See 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|

	binary_size	generate	max mem	batch_get 50	batch_put 50	batch_remove 50	upgrade
persistentmap_100	192_080	204_126	0	51_387	124_116	128_293	388_084
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	192_080	2_775_816	0	68_754	162_897	173_291	3_634_959
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	192_080	42_949_375	360_372	85_219	199_435	221_201	36_038_892
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	192_080	509_484_647	3_600_372	99_728	238_691	265_873	458_331_215
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	192_080	5_883_267_991	36_000_372	118_499	278_670	317_512	4_583_348_765
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|

	size	foldLeft	foldRight	mapfilter	map
persistentmap	100	19_623	19_535	89_915	26_278
persistentmap_baseline	100	92_138	93_745	169_663	29_048
persistentmap	1000	165_633	164_145	1_577_927	230_324
persistentmap_baseline	1000	888_169	899_681	3_556_717	257_766
persistentmap	10000	1_628_080	1_609_767	29_675_517	2_270_416
persistentmap_baseline	10000	19_529_035	19_640_763	43_314_564	2_544_359
persistentmap	100000	16_254_171	16_059_689	359_232_494	94_265_059
persistentmap_baseline	100000	195_212_923	196_318_318	505_815_170	132_210_500
persistentmap	1000000	162_559_221	876_570_432	4_198_876_914	942_589_235
persistentmap_baseline	1000000	1_952_082_879	1_963_098_142	5_763_869_513	1_322_035_748

GoPavel · 2024-10-14T17:15:16Z

Some results of our experiments we believe show where the Motoko compiler has the potential to perform better optimizations:

More inlining of private local functions: In the experiment we tried to avoid repetitive branches for different colors of nodes via moving common code into a scope-local function but got bad performance. Probably compiler could inline such functions.
We found that the last optimization (inlining colors) makes get slower which is surprising for us. The only difference in the get implementation is that instead of 2 constructors we have 3 (with duplicating branche bodies). I would expect the same performance, so probably this case requires more investigation and finally will give a clue as to how to improve compiler optimization.

src/PersistentOrderedMap.mo

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

crusso

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

crusso · 2024-10-23T23:32:50Z

src/PersistentOrderedMap.mo

+// 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.)


I guess we can delete the TODO now (all addressed right?)

crusso · 2024-10-23T23:33:37Z

src/PersistentOrderedMap.mo

+  ///
+  /// `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) {


Suggested change

public class MapOps<K>(compare : (K,K) -> O.Order) {

public class MapOps<K>(compare : (K, K) -> O.Order) {

crusso · 2024-10-23T23:37:48Z

src/PersistentOrderedMap.mo

+//  #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.


Suggested change

// 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?

Yes, we will make it. We hope it will not diminish performance much, but just in case we will check the benchmark again.

Agreed to make it in the 3rd Milestone (#662)

crusso · 2024-10-23T23:41:13Z

src/PersistentOrderedMap.mo

+    /// Cost of empty map creation
+    /// Runtime: `O(1)`.
+    /// Space: `O(1)`
+    public func empty<V>() : Map<K, V> = #leaf;


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?

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.

crusso · 2024-10-23T23:43:44Z

src/PersistentOrderedMap.mo

+    /// 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")]));


Did you decide not to rename rbMap to map throughout, or is that for a separate PR?

Yeah, we just did not rebase this branch yet because I was considering that there are probably some open discussions in the #654

crusso · 2024-10-24T01:00:10Z

src/PersistentOrderedMap.mo

+    /// 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);


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)?

Related: If we keep iter(rb, dir), can we move type Direction into MapOps?

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.

crusso · 2024-10-24T01:16:01Z

src/PersistentOrderedMap.mo

+          (#red (l, x, y, r))
+        };
+        case _ {
+          Debug.trap "RBTree.red"


Suggested change

Debug.trap "RBTree.red"

Debug.trap "PersistentOrderedMap.red"

Are there others that need fixing?

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.

github-actions bot added the external-contributor label Oct 14, 2024

GoPavel changed the title ~~Draft: Serokell: [Milestone-2] Optimize PersistenOrderedMap.mo~~ Serokell: [Milestone-2] Optimize PersistenOrderedMap.mo Oct 14, 2024

crusso reviewed Oct 15, 2024

View reviewed changes

src/PersistentOrderedMap.mo Outdated Show resolved Hide resolved

crusso reviewed Oct 15, 2024

View reviewed changes

src/PersistentOrderedMap.mo Outdated Show resolved Hide resolved

s-and-witch and others added 10 commits October 21, 2024 17:31

Extract persistent RBTree from mutable wrapper into a separate module

d97833b

No changes in logic so far, just simple refactoring

[DMS-66] fix order of method arguments

564f95a

Problem: now order is not consistent within new module and with old modules as well. Solution: make the map argument always go first

[DMS-76] Persistent ordered map unit testing

174e877

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.

[DMS-67] add property-based tests for PersistentOrderedMap

92925ab

[DMS-77] Add usage examples to the documentation

af994d6

Specify keys order in iterators

e040734

PersistentOrderedMap: fix typos

070673f

PersistentOrderedMap: fix imports in examples

ef13759

PerisistentOrderedMap: fix docs

d4b02eb

* 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

crusso reviewed Oct 24, 2024

View reviewed changes

GoPavel and others added 10 commits October 24, 2024 18:56

PersistentOrderedMap: fix newline in prop tests

26a33c1

Reuse iteration code and parametrize it with map walking function

35b48b1

Use direct recursion in folding functions

f665f1d

Implement mapFilter using foldLeft instad of iter

e15a3f2

Inline color field into map constructors

b29beea

Make internal module public

6dbaba6

Move leaf branch to the end of switch expression, as the most unlikely

97ebc9d

This PR didn't touch folds and map as benchmark showed us that it performs worse.

Move all operations inside a MapOps class

acd2add

Eliminate tuple in map constructors

d9e2cd1

PersistentOrderedMap: clean up comments + formatting typo

e0204cb

GoPavel force-pushed the milestone-2 branch from 9a0a264 to e0204cb Compare October 28, 2024 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serokell: [Milestone-2] Optimize PersistenOrderedMap.mo #664

Serokell: [Milestone-2] Optimize PersistenOrderedMap.mo #664

GoPavel commented Oct 14, 2024 •

edited

Loading

GoPavel commented Oct 14, 2024

crusso left a comment

crusso Oct 23, 2024

GoPavel Oct 24, 2024

crusso Oct 23, 2024

GoPavel Oct 28, 2024

crusso Oct 23, 2024 •

edited

Loading

GoPavel Oct 24, 2024

GoPavel Oct 28, 2024

crusso Oct 23, 2024

GoPavel Oct 24, 2024

crusso Oct 23, 2024

GoPavel Oct 24, 2024

GoPavel Oct 28, 2024

crusso Oct 24, 2024

crusso Oct 24, 2024 •

edited

Loading

GoPavel Oct 24, 2024

crusso Oct 24, 2024 •

edited

Loading

GoPavel Oct 24, 2024

	public class MapOps<K>(compare : (K,K) -> O.Order) {
	public class MapOps<K>(compare : (K, K) -> O.Order) {

	// 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.

	Debug.trap "RBTree.red"
	Debug.trap "PersistentOrderedMap.red"

Serokell: [Milestone-2] Optimize PersistenOrderedMap.mo #664

Are you sure you want to change the base?

Serokell: [Milestone-2] Optimize PersistenOrderedMap.mo #664

Conversation

GoPavel commented Oct 14, 2024 • edited Loading

Final performance comparison

Map comparison

Persistent map API

GoPavel commented Oct 14, 2024

crusso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusso Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusso Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusso Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GoPavel commented Oct 14, 2024 •

edited

Loading

crusso Oct 23, 2024 •

edited

Loading

crusso Oct 24, 2024 •

edited

Loading

crusso Oct 24, 2024 •

edited

Loading