Skip to content

Conversation

@silverjam
Copy link
Contributor

@silverjam silverjam commented Oct 8, 2021

Benchmarks, jemalloc without 128b minimum buffer size:

Benchmark #1: sbp2json <test_data/benchmark.sbp | json2sbp >/dev/null
  Time (mean ± σ):      1.755 s ±  0.076 s    [User: 2.689 s, System: 0.532 s]
  Range (min … max):    1.585 s …  1.922 s    50 runs

Using jemalloc with 128b minimum internal buffer size:

Benchmark #1: sbp2json <test_data/benchmark.sbp | json2sbp >/dev/null
  Time (mean ± σ):     718.1 ms ±  33.9 ms    [User: 1.085 s, System: 0.170 s]
  Range (min … max):   658.3 ms … 839.5 ms    50 runs

Using mimalloc with 128b minimum internal buffer size:

Benchmark #1: sbp2json <test_data/benchmark.sbp | json2sbp >/dev/null
  Time (mean ± σ):     642.9 ms ±  33.9 ms    [User: 989.1 ms, System: 160.9 ms]
  Range (min … max):   603.4 ms … 785.0 ms    50 runs

MiMalloc also has the benefit that it works for more than just Linux.

@silverjam silverjam force-pushed the silverjam/min-internal-buffer-size branch from a76fe1e to dc9e0d8 Compare October 8, 2021 05:06
@silverjam silverjam requested review from a team and notoriaga October 8, 2021 05:06

fn parse_unchecked<B: Buf>(buf: &mut B) -> Self {
let mut v = Vec::new();
let mut v = Vec::with_capacity(BUFLEN);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would something like this work?

let mut v = Vec::with_capacity(BUFLEN / mem::size_of::<T>());

it should over-allocate less often. but I think the buffer sizes would vary more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this which didn't seem to have any impact:

diff --git a/rust/sbp/src/wire_format.rs b/rust/sbp/src/wire_format.rs
index afdd09fd..3d3639d3 100644
--- a/rust/sbp/src/wire_format.rs
+++ b/rust/sbp/src/wire_format.rs
@@ -53,7 +53,7 @@ where
     }

     fn parse_unchecked<B: Buf>(buf: &mut B) -> Self {
-        let mut v = Vec::with_capacity(BUFLEN);
+        let mut v = Vec::with_capacity(64*(1+(mem::size_of::<T>() / 64)));
         while buf.remaining() >= T::MIN_LEN {
             v.push(T::parse_unchecked(buf));
         }

Benchmark:

Benchmark #1: sbp2json <test_data/benchmark.sbp | json2sbp >/dev/null
  Time (mean ± σ):     610.7 ms ±  18.9 ms    [User: 936.4 ms, System: 161.0 ms]
  Range (min … max):   561.0 ms … 684.1 ms    50 runs

Copy link
Contributor Author

@silverjam silverjam Oct 8, 2021

Choose a reason for hiding this comment

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

Smaller internal buffer size of 64 bytes was a bit slower:

Benchmark #1: sbp2json <test_data/benchmark.sbp | json2sbp >/dev/null
  Time (mean ± σ):     629.7 ms ±  18.2 ms    [User: 979.7 ms, System: 151.9 ms]
  Range (min … max):   584.6 ms … 678.3 ms    50 runs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Buffer size of 256:

Benchmark #1: sbp2json <test_data/benchmark.sbp | json2sbp >/dev/null
  Time (mean ± σ):     624.5 ms ±  18.7 ms    [User: 962.8 ms, System: 159.4 ms]
  Range (min … max):   587.7 ms … 698.3 ms    50 runs

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm interesting, I guess T is typically small anyway

env_logger = "0.8"
serde_json = "1.0"
structopt = "0.3"
mimalloc = { version = "*", default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to not have a version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No version is what the docs for the package recommended but they didn't give a rational, I can pin it

@notoriaga notoriaga force-pushed the silverjam/min-internal-buffer-size branch from 94a3832 to 5f86c8a Compare October 8, 2021 05:24
@notoriaga
Copy link
Contributor

notoriaga commented Oct 8, 2021

Oops was on the wrong branch for that push, I don't think it actually pushed anything though commit changed but I don't see what actually did

Jason Mobarak added 2 commits October 7, 2021 22:30
improves speed and also works on non-Linux platforms
improves perf by allow the allocator to do less work when reclaiming
memory
@silverjam silverjam force-pushed the silverjam/min-internal-buffer-size branch from 5f86c8a to 566faaa Compare October 8, 2021 05:30
@silverjam
Copy link
Contributor Author

Oops was on the wrong branch for that push, I don't think it actually pushed anything though commit changed but I don't see what actually did

no worries, I figured it out

@notoriaga notoriaga self-requested a review October 8, 2021 06:00
@silverjam silverjam merged commit 4221358 into master Oct 8, 2021
@silverjam silverjam deleted the silverjam/min-internal-buffer-size branch October 8, 2021 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants