Skip to content
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

use BTreeMap as backing store for objects #231

Merged
merged 2 commits into from
Dec 19, 2020

Conversation

tgolsson
Copy link
Contributor

@tgolsson tgolsson commented Dec 18, 2020

I've tried a dozen different methods here to see if I can improve some usages, and this seems to be the simplest and most stupid one: simply switch backing storage. I think the tradeoff here is that since BTreeMaps use cmp::Ord instead of cmp::Eq, we have to prefix search at each node of the BTree, so if there's a common prefix to members lookups will increase in cost.

Overall these results are expected: the brainfuck interpreter relies heavily on self so I'd expect it to improve, and the fibonnaci is pure recursive calls and should be unaffected. I cannot motivate why aoc_2020_{1a, 1b} got faster, however, nor that 11a does not improve... It might just be completely overshadowed by some other operation.

@udoprog it'd be interesting to see if you can replicate similar gains.

➜ cargo benchcmp before btreemap
 name             before ns/iter  btreemap ns/iter  diff ns/iter   diff %  speedup
 aoc_2020_11a     277,658,677     276,582,186         -1,076,491   -0.39%   x 1.00
 aoc_2020_1a      193,316         184,086                 -9,230   -4.77%   x 1.05
 aoc_2020_1b      581,259         572,379                 -8,880   -1.53%   x 1.02
 bf_fib           52,909,632      46,863,458          -6,046,174  -11.43%   x 1.13
 bf_hello_world   871,338         841,625                -29,713   -3.41%   x 1.04
 bf_hello_world2  9,531,777       8,708,816             -822,961   -8.63%   x 1.09
 bf_loopity       8,027,656       7,703,409             -324,247   -4.04%   x 1.04
 fib_15           313,758         317,448                  3,690    1.18%   x 0.99
 fib_20           3,520,573       3,556,172               35,599    1.01%   x 0.99

Methods I've also tried:

  • precompute keys inside objects
  • precompute all keys (yes, all. ConstValue, IrValue, you name it.)
  • VecMap (vector-map version)
  • Every possible hasher under the sun
  • BTreeMap + precomputed keys
  • ordered map

This version has:

  • least variable results (all the hash based ones are +-3% run-over-run, making most results statistically insignificant
  • little to no impact when objects are not used
  • maintains the current API
  • doesn't add any dependencies or performance cliffs

@tgolsson
Copy link
Contributor Author

Two more benchmarks:

➜ cargo benchcmp before btreemap2
 name             before ns/iter  btreemap2 ns/iter  diff ns/iter  diff %  speedup
 aoc_2020_11a     277,658,677     276,137,716          -1,520,961  -0.55%   x 1.01
 aoc_2020_1a      193,316         185,390                  -7,926  -4.10%   x 1.04
 aoc_2020_1b      581,259         598,370                  17,111   2.94%   x 0.97
 bf_fib           52,909,632      48,702,261           -4,207,371  -7.95%   x 1.09
 bf_hello_world   871,338         837,012                 -34,326  -3.94%   x 1.04
 bf_hello_world2  9,531,777       8,823,602              -708,175  -7.43%   x 1.08
 bf_loopity       8,027,656       7,753,124              -274,532  -3.42%   x 1.04
 fib_15           313,758         314,527                     769   0.25%   x 1.00
 fib_20           3,520,573       3,561,090                40,517   1.15%   x 0.99

➜ cargo benchcmp before btreemap3
 name             before ns/iter  btreemap3 ns/iter  diff ns/iter   diff %  speedup
 aoc_2020_11a     277,658,677     268,676,129          -8,982,548   -3.24%   x 1.03
 aoc_2020_1a      193,316         184,407                  -8,909   -4.61%   x 1.05
 aoc_2020_1b      581,259         596,956                  15,697    2.70%   x 0.97
 bf_fib           52,909,632      49,151,808           -3,757,824   -7.10%   x 1.08
 bf_hello_world   871,338         820,807                 -50,531   -5.80%   x 1.06
 bf_hello_world2  9,531,777       8,577,606              -954,171  -10.01%   x 1.11
 bf_loopity       8,027,656       7,486,971              -540,685   -6.74%   x 1.07
 fib_15           313,758         318,875                   5,117    1.63%   x 0.98
 fib_20           3,520,573       3,457,312               -63,261   -1.80%   x 1.02

Copy link
Collaborator

@udoprog udoprog left a comment

Choose a reason for hiding this comment

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

Very impressive! I think it's a great change for now. Real world workloads could of course end up behaving differently. But the outcomes with the existing benchmarks are about as close as we'll get to these for now.

Thank you!

use crate::{
FromValue, InstallWith, Item, Mut, Named, RawMut, RawRef, RawStr, Ref, ToValue,
UnsafeFromValue, Value, Vm, VmError,
};
use std::borrow;
use std::cmp;
use std::collections::BTreeMap as HashMap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe alias it to something more... neutral? :)

@tgolsson
Copy link
Contributor Author

Aye, will fix the name. Do you care to repro the improvement on your machine?

@tgolsson
Copy link
Contributor Author

I.e.

cargo +nightly bench > before
git fetch
git checkout ts/object-btreemap
cargo +nightly bench > after
cargo benchcmp before after

You might need to add me as a remote, not sure if fetches PRs.

@udoprog
Copy link
Collaborator

udoprog commented Dec 19, 2020

Can confirm:

 name             before ns/iter  after ns/iter  diff ns/iter   diff %  speedup 
 aoc_2020_11a     291,552,960     284,662,410      -6,890,550   -2.36%   x 1.02
 aoc_2020_1a      183,691         186,667               2,976    1.62%   x 0.98
 aoc_2020_1b      577,799         591,270              13,471    2.33%   x 0.98
 bf_fib           53,367,940      49,897,210       -3,470,730   -6.50%   x 1.07
 bf_hello_world   930,593         869,250             -61,343   -6.59%   x 1.07 
 bf_hello_world2  9,956,440       8,911,285        -1,045,155  -10.50%   x 1.12
 bf_loopity       8,620,805       7,959,440          -661,365   -7.67%   x 1.08
 fib_15           298,382         299,070                 688    0.23%   x 1.00
 fib_20           3,355,000       3,289,340           -65,660   -1.96%   x 1.02

@tgolsson
Copy link
Contributor Author

@udoprog Thanks! Updated the PR to re-expoert BTreeMap from the root crate like the other collections. Good to go?

@udoprog
Copy link
Collaborator

udoprog commented Dec 19, 2020

Yep! Merge whenever you feel like it.

@tgolsson tgolsson merged commit 8c0dff6 into rune-rs:main Dec 19, 2020
@udoprog udoprog added enhancement New feature or request changelog Issue has been added to the changelog labels Jan 19, 2021
@tgolsson tgolsson deleted the ts/object-btreemap branch March 3, 2023 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Issue has been added to the changelog enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants