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

Improve invariants and performance in Chain #3533

Merged
merged 10 commits into from
Aug 4, 2020
Merged

Conversation

johnynek
Copy link
Contributor

@johnynek johnynek commented Jul 27, 2020

I realized that if we keep a suptype of Chain to note that items are non-empty, we can (at no cost, I think) simplify the code and make it safer.

There is a minor bit of ugliness in Append because to make it binary compatible, we need to put back some methods that were there before. I went ahead and did this to satisfy mima even though these classes are private to data.

Side note: unless we really need to expose classes like this, I'm a big fan of keeping them totally private.

also is a possible replacement for #3373

@johnynek johnynek requested review from non and travisbrown July 27, 2020 20:19
@johnynek
Copy link
Contributor Author

I also avoid writing case Empty => since that triggers a call to def equals which triggers a call to iterator. I managed to hit an infinite loop in one version of the code, but no reason to hit that for a sealed private class, we know Empty by exclusion and it is the least common case I would expect.

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2020

Codecov Report

Merging #3533 into master will decrease coverage by 0.20%.
The diff coverage is 88.81%.

@@            Coverage Diff             @@
##           master    #3533      +/-   ##
==========================================
- Coverage   91.32%   91.12%   -0.21%     
==========================================
  Files         386      386              
  Lines        8592     8989     +397     
  Branches      269      272       +3     
==========================================
+ Hits         7847     8191     +344     
- Misses        745      798      +53     

LukaJCB
LukaJCB previously approved these changes Jul 27, 2020
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

This is really cool, thanks!

@johnynek
Copy link
Contributor Author

While looking at this I found a performance improvement by not using an internal ArrayBuffer of 16% for large Chains. This is for iterator, which is used broadly. foldLeft exercises iterator.

This change:

[info] Benchmark                           Mode  Cnt         Score        Error  Units
[info] ChainBench.foldLeftLargeCatenable  thrpt    5        56.035 ±      0.661  ops/s
[info] ChainBench.foldLeftLargeChain      thrpt    5       199.145 ±      1.080  ops/s
[info] ChainBench.foldLeftLargeList       thrpt    5       158.537 ±      7.086  ops/s
[info] ChainBench.foldLeftLargeOldChain   thrpt    5       148.596 ±      1.560  ops/s
[info] ChainBench.foldLeftLargeVector     thrpt    5        77.262 ±      0.680  ops/s
[info] ChainBench.foldLeftSmallCatenable  thrpt    5  11846407.099 ±  22880.990  ops/s
[info] ChainBench.foldLeftSmallChain      thrpt    5  48157018.969 ± 246883.975  ops/s
[info] ChainBench.foldLeftSmallList       thrpt    5  55998179.055 ± 241976.102  ops/s
[info] ChainBench.foldLeftSmallOldChain   thrpt    5  55485668.350 ±  90607.034  ops/s
[info] ChainBench.foldLeftSmallVector     thrpt    5  14805536.562 ± 749006.508  ops/s


master:
[info] Benchmark                           Mode  Cnt         Score        Error  Units
[info] ChainBench.foldLeftLargeCatenable  thrpt    5        57.285 ±      1.891  ops/s
[info] ChainBench.foldLeftLargeChain      thrpt    5       170.942 ±      2.281  ops/s
[info] ChainBench.foldLeftLargeList       thrpt    5       159.709 ±      4.833  ops/s
[info] ChainBench.foldLeftLargeOldChain   thrpt    5       145.871 ±      7.981  ops/s
[info] ChainBench.foldLeftLargeVector     thrpt    5        76.742 ±      1.634  ops/s
[info] ChainBench.foldLeftSmallCatenable  thrpt    5  11798663.388 ± 606629.280  ops/s
[info] ChainBench.foldLeftSmallChain      thrpt    5  47750078.331 ± 473361.638  ops/s
[info] ChainBench.foldLeftSmallList       thrpt    5  56818234.798 ± 420088.798  ops/s
[info] ChainBench.foldLeftSmallOldChain   thrpt    5  63018070.265 ± 515461.221  ops/s
[info] ChainBench.foldLeftSmallVector     thrpt    5  14840988.935 ± 381582.742  ops/s

@LukaJCB can you look again?

LukaJCB
LukaJCB previously approved these changes Jul 27, 2020
@johnynek johnynek changed the title Improve invariants in Chain Improve invariants and performance in Chain Jul 27, 2020
barambani
barambani previously approved these changes Jul 28, 2020
@johnynek
Copy link
Contributor Author

johnynek commented Aug 3, 2020

@barambani could you take another look (merged with master, a few minor cleanups).

@barambani
Copy link
Contributor

LGTM on green. I think the failures are due to the update to scala 2.13.3. There are few missing parentheses on 'next' calls that now makes the build fail.

@johnynek
Copy link
Contributor Author

johnynek commented Aug 3, 2020

Thanks @barambani I think I fixed it. Sorry for so many revisions here. :/ one more 👍

Copy link
Contributor

@barambani barambani left a comment

Choose a reason for hiding this comment

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

Oh don't say it. It' my pleasure looking at your PRs.

@johnynek johnynek merged commit 07e23b1 into master Aug 4, 2020
@travisbrown travisbrown added this to the 2.2.0-RC3 milestone Aug 15, 2020
@larsrh larsrh deleted the oscar/chain_nonempty branch September 19, 2020 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants