Skip to content

Avoid frollapply() stack overflows and protect limits#3994

Merged
mattdowle merged 5 commits intoRdatatable:masterfrom
DavisVaughan:frollapply-fixes
Dec 8, 2019
Merged

Avoid frollapply() stack overflows and protect limits#3994
mattdowle merged 5 commits intoRdatatable:masterfrom
DavisVaughan:frollapply-fixes

Conversation

@DavisVaughan
Copy link
Copy Markdown
Contributor

Closes #3993

The following three examples from #3993 now run without issue, no segfaults!

library(data.table)

xx <- frollapply(1, rep(1L, 1e5), identity)

yy <- frollapply(1, rep(1L, 1e6), identity)

zz <- frollapply(as.list(rep(1, 1e6)), 1, identity)

If someone else wants to take over adding tests (if you want them), please go ahead. I'm not familiar with how you all test the package.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 25, 2019

Codecov Report

Merging #3994 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3994      +/-   ##
=========================================
- Coverage    99.4%   99.4%   -0.01%     
=========================================
  Files          72      72              
  Lines       13652   13650       -2     
=========================================
- Hits        13571   13569       -2     
  Misses         81      81
Impacted Files Coverage Δ
src/frollR.c 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8dd0db...9607681. Read the comment docs.

@jangorecki
Copy link
Copy Markdown
Member

Thanks for fixes. The second commit is a nice change. I kept all objects pre-allocated to align to the code of other rolling functions, but because roll apply will not run in parallel we can safely override those variables inside the loop. Will add tests.

@jangorecki
Copy link
Copy Markdown
Member

Tests were added but disabled by default as they require 1.6 GB, 16 GB and 16GB memory. There is no way to tests those failures in a lightweight way.

@jangorecki jangorecki added this to the 1.12.7 milestone Nov 24, 2019
@mattdowle mattdowle merged commit 3d2df7e into Rdatatable:master Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

frollapply() issues

3 participants