Serokell: [Milestone-1] New PersistentOrderedMap.mo lib #654
Conversation
|
Dear @GoPavel, In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA. If you decide to agree with it, please visit this issue and read the instructions there. Once you have signed it, re-trigger the workflow on this PR to see if your code can be merged. — The DFINITY Foundation |
GoPavel
force-pushed
the
milestone-1
branch
from
740c391
to
41b6516
Compare
GoPavel
changed the title
GoPavel
changed the title
| /// with `Map` to maintain invariant that comparator did not changed. | ||
| /// | ||
| /// `MapOps` contains methods that require `compare` internally: | ||
| /// operations that may reshape a `Map` or should find something. |
There was a problem hiding this comment.
| /// operations that may reshape a `Map` or should find something. | |
| /// operations that update, traverse or search a map. |
There was a problem hiding this comment.
I think traverse doesn't require comp (since we keep tree structure) so it's outside of the MapOps.
So I think it should be
| /// operations that may reshape a `Map` or should find something. | |
| /// operations that update or search a map. |
There was a problem hiding this comment.
I guess that's a UX question - should we put all operations in MapOps, even if they don't require the comparison, just for uniformity? I guess we could also do that later easily enough. Something to think about.
There was a problem hiding this comment.
should we put all operations in MapOps
I also thought about this. We were going to measure its overhead on a benchmark and then decide. If there is no difference I would also put it there.
However for now some modules like RBTree or TrieMap provide some functionality as a module-function, not as a member-function e.g. map or iter. So we thought that the current solution is not very inconsistent.
There was a problem hiding this comment.
We did performance experiments:
- Comparing inlined internal functions and the current version (all calls through
MapOps): [DMS-68] Inline internal functions into MapOps object serokell/motoko-base#21 - Comparing calling methods through MapOps and through the
Internalmodule passingcompmanually: [DMS-68] Compare function calls from MapOps with direct function call serokell/motoko-base#20
The 1st experiment shows that the inlined version has more or less the same performance (<1% difference).
The 2nd one shows that a static function call is faster than a similar one from MapOps on 1-4% percent.
Thus we can say that static calls are less convenient but faster, so probably it makes sense to keep the option ?
We can add the rest of the functions into MapOps and make the Internal module public.
OR keep the current version as compromised
WDYT?
There was a problem hiding this comment.
So I guess we decide to put all operations in MapOps and leave Internal private for now at a 1-4% perf hit.
Does the perf hit disappear entirely if we inline the Internal functions in MapOps, or am I misunderstanding?
GoPavel
changed the title
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 |
| /// with `Map` to maintain invariant that comparator did not changed. | ||
| /// | ||
| /// `MapOps` contains methods that require `compare` internally: | ||
| /// operations that may reshape a `Map` or should find something. |
There was a problem hiding this comment.
So I guess we decide to put all operations in MapOps and leave Internal private for now at a 1-4% perf hit.
Does the perf hit disappear entirely if we inline the Internal functions in MapOps, or am I misunderstanding?
We have tried it, and it doesn't affect the benchmark at all. See serokell#21 |
This is an MR for the 1st Milestone of Serokell's grant about improving Motoko's base library.
Within it, we are introducing a new functional implementation of the map data structure based on the current
RBTree.mo.Content of the MR:
RBTree.momapfoldLeft,foldRightmapFilterkeys,vals?V -> V