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 performance #29

Merged
merged 2 commits into from
Jul 11, 2022
Merged

Improve performance #29

merged 2 commits into from
Jul 11, 2022

Conversation

halhen
Copy link

@halhen halhen commented Jul 10, 2022

Following your request at #14 (comment), I took a stab at speeding generateCombinationsImpl up a bit. My results are 3-4X speedup; the majority of the time is now spent paste():ing combination names.

Not an earth-shattering difference, but also not nothing perhaps?

Unit tests pass, but please verify that I've broken no assumptions.

5950X, 128 GB RAM, Fedora Linux 36, R 4.1.3, v1.11.0 / commit 12f306f

Before this PR:

> microbenchmark::microbenchmark(
+     without_store_elems = upsetjs() %>%
+         fromDataFrame(toyset_2, c_type="distinctIntersection", store.elems=FALSE, limit = 40),
+     with_store_elems = upsetjs() %>%
+         fromDataFrame(toyset_2, c_type="distinctIntersection", store.elems=TRUE, limit = 40),
+     times = 10L
+ )
Unit: milliseconds
                expr      min       lq     mean   median       uq      max neval
 without_store_elems 825.4124 1044.876 1047.124 1078.969 1105.553 1126.978    10
    with_store_elems 943.7230 1031.711 1047.950 1058.206 1082.389 1096.377    10

With this PR:

> microbenchmark::microbenchmark(
+     without_store_elems = upsetjs() %>%
+         fromDataFrame(toyset_2, c_type="distinctIntersection", store.elems=FALSE, limit = 40),
+     with_store_elems = upsetjs() %>%
+         fromDataFrame(toyset_2, c_type="distinctIntersection", store.elems=TRUE, limit = 40),
+     times = 10L
+ )
Unit: milliseconds
                expr      min       lq     mean   median       uq      max neval
 without_store_elems 239.3860 243.9979 251.5668 252.5741 258.3372 261.2554    10
    with_store_elems 305.0873 319.4910 342.5158 328.5697 335.9419 472.6493    10

Makes calling extractCombinationsImpl about 3x faster
Speed savings with store.elems = FALSE are ~20%
@sgratzl sgratzl changed the base branch from main to develop July 11, 2022 12:31
@sgratzl sgratzl self-assigned this Jul 11, 2022
@sgratzl sgratzl added the enhancement New feature or request label Jul 11, 2022
@sgratzl sgratzl merged commit ad1acbe into upsetjs:develop Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants