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

Decrease deserialization complexity from quadratic to linear #349

Merged
merged 4 commits into from Oct 28, 2019

Conversation

@est31
Copy link
Contributor

est31 commented Oct 25, 2019

Fixes #342.

In particular, see my comment #342 (comment)

Values that I recorded on my machine for running the code measure_time(n, |i| format!("[header_no_{}]\n", i)) function for varying n:

benchmark optimizations before this PR after this PR
1k no 170ms 48ms
10k no 14 191ms 483ms
100k no n/a 5 077ms
1k yes 5ms 4ms
10k yes 311ms 39ms
100k yes 63 850ms 364ms

You can nicely see how before this PR, a 10x increase in data meant a 100x increase in time spent, while after the PR it only means a 10x increase.

Also add regression test
@est31 est31 force-pushed the est31:speedup branch from 0140d4a to 9a38a82 Oct 25, 2019
@est31 est31 changed the title Increase deserialization speed from quadratic to linear Increase deserialization speed from quadratic time to linear Oct 25, 2019
@est31 est31 changed the title Increase deserialization speed from quadratic time to linear Decrease deserialization complexity from quadratic to linear Oct 25, 2019
Copy link
Owner

alexcrichton left a comment

Thanks for this @est31!

Can this also include documentation as to what these two intermediate tables are? Either on the struct fields or on the functions that construct them.

src/de.rs Outdated
.and_then(|entries| {
let start = entries
.binary_search(&self.cur)
.unwrap_or_else(std::convert::identity);

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Oct 28, 2019

Owner

I'd personally prefer if this were unwrap_or_else(|i| i)

@est31 est31 force-pushed the est31:speedup branch from 96c3ca8 to fa61d1a Oct 28, 2019
@est31 est31 force-pushed the est31:speedup branch from fa61d1a to 37484d5 Oct 28, 2019
@est31 est31 requested a review from alexcrichton Oct 28, 2019
@alexcrichton alexcrichton merged commit c049b7a into alexcrichton:master Oct 28, 2019
5 checks passed
5 checks passed
Test (stable)
Details
Test (beta)
Details
Test (nightly)
Details
Rustfmt
Details
Publish Documentation
Details
@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Oct 28, 2019

👍

@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Oct 28, 2019

It looks like the tests added here may be causing a spurious failure on CI?

@est31

This comment has been minimized.

Copy link
Contributor Author

est31 commented Oct 28, 2019

Huh, that's weird. It works on my machine but it failed on CI previously as well. As a result I increased the tolerance value, but it seems further increases are needed. The difference is quite large. I can maybe increase the sample size, which should lead to less noise. And maybe increase the multiplier as well to allow for even larger tolerances. Does that sound reasonable?

@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Oct 28, 2019

CI machines (VMs) tend to be extremely noisy in terms of measurements, so I think it's fine to perhaps remove the test and move it to a benchmark which can be manually tracked over time. This is pretty unlikely to regress.

est31 added a commit to est31/toml-rs that referenced this pull request Oct 28, 2019
est31 added a commit to est31/toml-rs that referenced this pull request Oct 28, 2019
CI environments can be noisy and while the test worked great
locally on my machine, it didn't on the CI environment.
This replaces the test with a (manually tracked) benchmark.
As per alexcrichton#349 (comment)
est31 added a commit to est31/toml-rs that referenced this pull request Oct 28, 2019
CI environments can be noisy and while the test worked great
locally on my machine, it didn't on the CI environment.
This replaces the test with a (manually tracked) benchmark.
As per alexcrichton#349 (comment)
alexcrichton added a commit that referenced this pull request Oct 29, 2019
CI environments can be noisy and while the test worked great
locally on my machine, it didn't on the CI environment.
This replaces the test with a (manually tracked) benchmark.
As per #349 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.