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 delete & insert performance #94

Merged
merged 3 commits into from Oct 12, 2020

Conversation

ChrisPenner
Copy link
Collaborator

@ChrisPenner ChrisPenner commented Oct 4, 2020

  • After Remove dependency on containers #93 ,insert = union (one x) gives O(n) inserts, still not great; but better than O(nlogn)
  • delete: Using toSortedTriples, which is an O(n) way to get a sorted list of triples, we can simply filter out the offending element and rebuild in O(n) total time

Addresses #57

This should be rebased on top of #93 once it's merged,

In the meantime; these are the relevant commits from just this PR

Copy link

@hint-man hint-man bot left a comment

Choose a reason for hiding this comment

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

Do you know why your PR is still not approved? Because I chose not to approve it. But they will.

src/Data/TypeRepMap/Internal.hs Outdated Show resolved Hide resolved
src/Data/TypeRepMap/Internal.hs Outdated Show resolved Hide resolved
src/Data/TypeRepMap/Internal.hs Outdated Show resolved Hide resolved
@ChrisPenner ChrisPenner force-pushed the improve-delete-insert-perf branch 4 times, most recently from 2356887 to 1fc35af Compare October 4, 2020 00:46
@chshersh chshersh added enhancement Hacktoberfest https://hacktoberfest.digitalocean.com/ hacktoberfest-accepted Accept contributions during Hacktoberfest labels Oct 4, 2020
@ChrisPenner ChrisPenner force-pushed the improve-delete-insert-perf branch 2 times, most recently from dfb1681 to 6abb108 Compare October 11, 2020 01:15
@ChrisPenner
Copy link
Collaborator Author

This PR has been rebased and is ready for review 👍

@ChrisPenner
Copy link
Collaborator Author

I'll run some benchmarking on this too and report back.

@ChrisPenner
Copy link
Collaborator Author

ChrisPenner commented Oct 11, 2020

Here're the relevant comparisons between current master and this branch:;

TLDR;

  • the 10 element insert benchmarks are probably not useful, they have ~90% variance due to outliers and are too small to be effective. In fact, I'd recommend deleting them, since in this case they show only a very minor improvement, whereas the "big" lookup benchmark shows a massive improvement. They're more likely to mislead people than to help. Or perhaps we simply need to increase the volume of inserts.

  • Good news! The mean lookup time from the big map goes from 335 ms down to 10 ms; that means the new version is 3% of the work in practice 😄

  • It appears the benchmarks are a bit misleading; the "update/1 element to big map/CacheMap" benchmark appears to actually "insert" into a big map 10 times; regardless it's definitely relevant to this PR; since it goes from 3.3s down to 97ms!

Insert

master

benchmarking insert/10 elements to empty/CacheMap
time                 50.55 μs   (47.29 μs .. 53.36 μs)
                     0.974 R²   (0.966 R² .. 0.985 R²)
mean                 49.18 μs   (47.44 μs .. 52.17 μs)
std dev              6.973 μs   (6.348 μs .. 8.210 μs)
variance introduced by outliers: 91% (severely inflated)

benchmarking insert/1 element to big map/CacheMap
time                 334.6 ms   (311.4 ms .. 353.3 ms)
                     0.999 R²   (NaN R² .. 1.000 R²)
mean                 341.2 ms   (337.2 ms .. 345.6 ms)
std dev              5.298 ms   (1.587 ms .. 7.033 ms)
variance introduced by outliers: 19% (moderately inflated)

benchmarking update/1 element to big map/CacheMap
time                 3.296 s    (3.254 s .. 3.360 s)
                     1.000 R²   (1.000 R² .. NaN R²)
mean                 3.327 s    (3.300 s .. 3.374 s)
std dev              44.64 ms   (5.056 ms .. 56.45 ms)
variance introduced by outliers: 19% (moderately inflated)

new

benchmarking insert/10 elements to empty/CacheMap
time                 46.18 μs   (43.57 μs .. 49.69 μs)
                     0.973 R²   (0.963 R² .. 0.984 R²)
mean                 47.63 μs   (45.77 μs .. 50.72 μs)
std dev              8.029 μs   (6.576 μs .. 10.69 μs)
variance introduced by outliers: 94% (severely inflated)

benchmarking insert/1 element to big map/CacheMap
time                 9.916 ms   (9.607 ms .. 10.22 ms)
                     0.995 R²   (0.992 R² .. 0.997 R²)
mean                 9.788 ms   (9.682 ms .. 9.989 ms)
std dev              395.2 μs   (335.5 μs .. 545.5 μs)
variance introduced by outliers: 16% (moderately inflated)

benchmarking update/1 element to big map/CacheMap
time                 97.07 ms   (93.20 ms .. 100.8 ms)
                     0.997 R²   (0.990 R² .. 1.000 R²)
mean                 96.27 ms   (94.55 ms .. 98.69 ms)
std dev              3.660 ms   (2.071 ms .. 5.689 ms)

We don't currently have delete benchmarks, so I'll look at adding those, but given the implementation I'd expect the same level of speedup.

Looking over these benchmarks it may be a good Hacktoberfest project for someone, although it's certainly true that benchmarks are surprisingly difficult to get right 😬

Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

@ChrisPenner That's impressive work! Small changes, but huge performance boost 🚄 Very nice benchmark results! And if you think that some of the benchmarks can be improved, I would encourage you to open a separate issue and discuss possible improvements 👍

delete = fromTriples . deleteByFst (typeFp @a) . toTriples
delete m
-- Lookups are fast, so check if we even have the element first.
| not (member @a m) = m
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, that's an interesting idea! Very nice optimization 👍
I wonder, if we can do something like that for insert? For example, if the element exists, we can just update the array cell and copy the whole array instead of moving from one ordering to another.

But I guess, this will require some work, and probably be done under a separate issue 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Created a separate issue for future insert improvements: #99

@chshersh chshersh merged commit 91991f9 into kowainik:master Oct 12, 2020
@chshersh chshersh added this to the v0.4.0.0: Boost milestone Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Hacktoberfest https://hacktoberfest.digitalocean.com/ hacktoberfest-accepted Accept contributions during Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants