dj8yfo
commented
1 year ago There are only tests here, but no actual implementation that was in #160. Is it on purpose?
Nope, it's just draft. Pushed it to periodically see feedback from ci.
Closed dj8yfo closed 1 year ago
dj8yfo
commented
1 year ago There are only tests here, but no actual implementation that was in #160. Is it on purpose?
Nope, it's just draft. Pushed it to periodically see feedback from ci.
dj8yfo
commented
1 year ago master and master2 correspond to same code 0f3e8c6fd80e4b9e60476b53d05499dcd4e7bda5 , warm-up-time 6, measurement-time 45
master and master2 for .*10000/ benches is within 4%master and master2 for all benches is within 8%critcmp master baseline_canonicity -t 5, baseline_canonicity is this branch without de_strict_order enabled
group baseline_canonicity master
----- ------------------- ------
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10/borsh_de/ 1.00 1029.6±11.21ns ? ?/sec 1.13 1166.6±10.50ns ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_1000/borsh_de/ 1.00 146.2±0.94µs ? ?/sec 1.55 227.2±1.35µs ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10000/borsh_de/ 1.00 1675.0±13.88µs ? ?/sec 1.65 2.8±0.02ms ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/ 1.00 352.6±1.54ns ? ?/sec 1.18 415.8±1.23ns ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/ 1.00 38.0±0.12µs ? ?/sec 2.21 84.0±0.41µs ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/ 1.00 405.1±2.23µs ? ?/sec 2.63 1064.4±6.20µs ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10/borsh_de/ 1.00 620.4±20.01ns ? ?/sec 1.08 667.0±16.22ns ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10/borsh_de/ 1.00 1643.7±18.87ns ? ?/sec 1.27 2.1±0.02µs ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_1000/borsh_de/ 1.00 193.7±1.23µs ? ?/sec 1.44 279.6±1.62µs ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10000/borsh_de/ 1.00 2.2±0.02ms ? ?/sec 1.33 2.9±0.10ms ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/ 1.00 510.4±2.21ns ? ?/sec 1.40 714.3±4.10ns ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/ 1.00 46.0±0.25µs ? ?/sec 1.77 81.4±0.22µs ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/ 1.00 487.6±3.85µs ? ?/sec 1.57 763.7±3.31µs ? ?/sec
frol
commented
1 year ago baseline
masterandmaster2correspond to same code
I don't see any other references to master2. Was it intended to be included as one more column in the benchmark table?
critcmp master baseline_canonicity -t 5,baseline_canonicityis this branch withoutde_strict_orderenabled
Do I read it correctly that performance degraded even without de_strict_order getting enabled? And this is why you did https://github.com/near/borsh-rs/pull/162/commits/02ea30530abbbeb984af2c61186344c4a87237ee ?
What is the current state? It sounds like you are still working on it, and we are not ready to merge, so I will turn this PR into draft (please, do it yourself next time to communicate this state). Turn it back "ready for review" once you are ready.
dj8yfo
commented
1 year ago This is a renewed summary of current pr. Summary of changes:
HashSet default behaviour - unchangedHashMap default behaviour -> changed to parse Vec<(K, V)> firstBTreeSet default behaviour - unchangedBTreeMap default behaviour -> changed to parse Vec<(K, V)> firstHashSet/HashMap/BTreeSet/BTreeMap got #[cfg(feature = "de_strict_order")] addonBelow is an example of expected errors in measurements (warm-up-time 30, measurement-time 210):
# compare master with itself; error up to 7%
❯ critcmp master master2 -t 6
group master master2
----- ------ -------
alloc::collections::btree::set::BTreeSet<alloc::string::String>_1000/borsh_de/ 1.07 80.7±2.65µs ? ?/sec 1.00 75.4±1.36µs ? ?/secNext, is difference between master and baseline_canonicity (branch of pr with default features):
❯ critcmp master baseline_canonicity
group baseline_canonicity master
----- ------------------- ------
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10/borsh_de/ 1.08 1254.0±2.68ns ? ?/sec 1.00 1156.9±3.08ns ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_1000/borsh_de/ 1.00 154.2±0.29µs ? ?/sec 1.46 224.8±0.71µs ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10000/borsh_de/ 1.00 1654.9±5.81µs ? ?/sec 1.65 2.7±0.01ms ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/ 1.00 369.2±1.90ns ? ?/sec 1.12 414.9±1.39ns ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/ 1.00 39.0±0.09µs ? ?/sec 2.16 84.4±0.54µs ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/ 1.00 413.6±0.72µs ? ?/sec 2.52 1042.4±7.39µs ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10/borsh_de/ 1.11 718.2±8.19ns ? ?/sec 1.00 645.1±2.90ns ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_1000/borsh_de/ 1.00 77.3±0.32µs ? ?/sec 1.04 80.7±2.65µs ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10000/borsh_de/ 1.03 887.7±1.92µs ? ?/sec 1.00 863.7±4.21µs ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_10/borsh_de/ 1.00 172.2±3.60ns ? ?/sec 1.02 176.0±3.02ns ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_1000/borsh_de/ 1.02 18.3±0.22µs ? ?/sec 1.00 18.0±0.05µs ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_10000/borsh_de/ 1.02 188.1±0.65µs ? ?/sec 1.00 184.0±0.62µs ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10/borsh_de/ 1.00 1590.3±6.73ns ? ?/sec 1.30 2.1±0.05µs ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_1000/borsh_de/ 1.00 198.2±5.08µs ? ?/sec 1.41 279.9±0.86µs ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10000/borsh_de/ 1.00 2.2±0.05ms ? ?/sec 1.31 2.8±0.01ms ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/ 1.00 521.8±9.18ns ? ?/sec 1.36 707.5±1.65ns ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/ 1.00 47.3±0.13µs ? ?/sec 1.70 80.4±0.48µs ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/ 1.00 505.3±1.18µs ? ?/sec 1.49 751.3±2.40µs ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_10/borsh_de/ 1.04 1066.7±1.40ns ? ?/sec 1.00 1024.6±2.85ns ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_1000/borsh_de/ 1.03 119.9±2.29µs ? ?/sec 1.00 116.1±1.33µs ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_10000/borsh_de/ 1.00 1255.9±14.32µs ? ?/sec 1.01 1272.7±40.84µs ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_10/borsh_de/ 1.13 381.0±30.18ns ? ?/sec 1.00 335.7±2.65ns ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_1000/borsh_de/ 1.00 29.5±0.89µs ? ?/sec 1.00 29.6±0.27µs ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_10000/borsh_de/ 1.00 306.3±1.98µs ? ?/sec 1.02 312.9±1.26µs ? ?/sec
This list has 2 anomalies, which must be due to some intermittent error on specific hardware, which i couldn't get rid of even with prolonged measument times.
group baseline_canonicity master
----- ------------------- ------
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10/borsh_de/ 1.11 718.2±8.19ns ? ?/sec 1.00 645.1±2.90ns ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_10/borsh_de/ 1.13 381.0±30.18ns ? ?/sec 1.00 335.7±2.65ns ? ?/secBTreeSet and HashSet implementations with default features hasn't changed, so it must be 0% instead of 11% and 13%.
Finally, raw unadjusted data of difference between baseline_canonicity and canonicity_de_strict_order (branch of pr with de_strict_order feature enabled):
❯ critcmp baseline_canonicity canonicity_de_strict_order
group baseline_canonicity canonicity_de_strict_order
----- ------------------- --------------------------
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10/borsh_de/ 1.00 1254.0±2.68ns ? ?/sec 1.01 1268.7±3.63ns ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_1000/borsh_de/ 1.00 154.2±0.29µs ? ?/sec 1.00 154.9±5.27µs ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10000/borsh_de/ 1.00 1654.9±5.81µs ? ?/sec 1.02 1689.3±4.42µs ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/ 1.01 369.2±1.90ns ? ?/sec 1.00 366.5±1.37ns ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/ 1.00 39.0±0.09µs ? ?/sec 1.00 39.1±0.07µs ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/ 1.00 413.6±0.72µs ? ?/sec 1.01 415.9±0.78µs ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10/borsh_de/ 1.00 718.2±8.19ns ? ?/sec 1.02 734.4±5.56ns ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_1000/borsh_de/ 1.00 77.3±0.32µs ? ?/sec 1.08 83.5±0.32µs ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10000/borsh_de/ 1.00 887.7±1.92µs ? ?/sec 1.06 936.8±5.02µs ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_10/borsh_de/ 1.00 172.2±3.60ns ? ?/sec 1.07 184.7±1.41ns ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_1000/borsh_de/ 1.00 18.3±0.22µs ? ?/sec 1.07 19.6±0.34µs ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_10000/borsh_de/ 1.00 188.1±0.65µs ? ?/sec 1.06 198.5±0.44µs ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10/borsh_de/ 1.01 1590.3±6.73ns ? ?/sec 1.00 1569.7±9.70ns ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_1000/borsh_de/ 1.00 198.2±5.08µs ? ?/sec 1.00 197.9±0.43µs ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10000/borsh_de/ 1.00 2.2±0.05ms ? ?/sec 1.05 2.3±0.05ms ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/ 1.00 521.8±9.18ns ? ?/sec 1.05 548.0±25.73ns ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/ 1.00 47.3±0.13µs ? ?/sec 1.13 53.3±2.53µs ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/ 1.00 505.3±1.18µs ? ?/sec 1.01 511.3±1.67µs ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_10/borsh_de/ 1.02 1066.7±1.40ns ? ?/sec 1.00 1043.3±2.58ns ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_1000/borsh_de/ 1.01 119.9±2.29µs ? ?/sec 1.00 119.2±1.13µs ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_10000/borsh_de/ 1.00 1255.9±14.32µs ? ?/sec 1.02 1275.2±1.75µs ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_10/borsh_de/ 1.14 381.0±30.18ns ? ?/sec 1.00 334.2±2.69ns ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_1000/borsh_de/ 1.00 29.5±0.89µs ? ?/sec 1.04 30.7±0.36µs ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_10000/borsh_de/ 1.00 306.3±1.98µs ? ?/sec 1.26 385.9±0.37µs ? ?/sec
@dj8yfo Do I read it right, that:
de_strict_order feature adds a negligible performance penalty almost indistinguishable from the measurement noiseVec first bring a significant performance improvementThis is really nice!
Resolves #63