Skip to content

gsum, gmean, gfirst, glast, g[ for complex vectors#3692

Merged
mattdowle merged 23 commits intomasterfrom
cplx_gforce
Jul 17, 2019
Merged

gsum, gmean, gfirst, glast, g[ for complex vectors#3692
mattdowle merged 23 commits intomasterfrom
cplx_gforce

Conversation

@MichaelChirico
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico commented Jul 10, 2019

part of #3690

Haven't added any tests yet but the basic examples work. Will return to this.

@MichaelChirico
Copy link
Copy Markdown
Member Author

Hmm I'm segfaulting gsum, will have to take a closer look

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 13, 2019

Codecov Report

Merging #3692 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3692      +/-   ##
==========================================
+ Coverage   98.24%   98.26%   +0.01%     
==========================================
  Files          69       69              
  Lines       13122    13243     +121     
==========================================
+ Hits        12892    13013     +121     
  Misses        230      230
Impacted Files Coverage Δ
src/gsumm.c 97.28% <100%> (+0.38%) ⬆️
src/coalesce.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 99d8fb1...4f2a97d. Read the comment docs.

@mattdowle mattdowle added the WIP label Jul 16, 2019
@mattdowle mattdowle changed the title WIP: Implement gsum, gmean, gfirst, glast, g[ for complex vectors, part of… gsum, gmean, gfirst, glast, g[ for complex vectors Jul 16, 2019
@mattdowle mattdowle added this to the 1.12.4 milestone Jul 17, 2019
@mattdowle
Copy link
Copy Markdown
Member

mattdowle commented Jul 17, 2019

For future reference, "torture by Rstrict yielded" means using R-devel built with UBSAN, ASAN and strict-barrier (see section in CRAN_Release.cmd) and this dev cycle :

R CMD build .
Rdevel-strict CMD INSTALL data.table_1.12.3.tar.gz  # with -O0 -g
Rdevel-strict
require(data.table)
DT = data.table(A=1:5, B=-3i, C=2147483647L)
gctorture2(step=100)
DT[, sum(B), by=A%%2L, verbose=TRUE]

It was an overwrite into R's heap which is hard to debug since all you get is a crash in the next gc which could be a long time after the root cause corruption occurred. So the first thing to do is to torture so that you get closer to where the root cause corruption happens. This also enabled me to reproduce locally on 64bit Linux (it was nothing to do with 32bit Windows, it's just 32bit Windows is often most sensitive to this type of corruption as it seems to gc more, perhaps). In this case it was the gx = R_alloc() that had not been allocated big enough (sizeof(double) was the largest type and now it is sizeof(Rcomplex)). When it's an overwrite to memory in R's own heap (as R_alloc returns) ASAN doesn't track that. ASAN tracks malloc and calloc. R's heap (R_alloc) is doing its own memory management so if our C code overwrites to that, those overwrites aren't detected by ASAN. But by using torture and Rprintf I was able to hone in on the area of code where the overwrite was happening and then by looking at the code and comments I spotted the R_alloc sizeof too small since I knew to look for things like that. In other words, by "yielded" I didn't mean that the tool just took me straight there; I mean it helped a lot. Fresh eyes after a night's sleep also helped.

The good thing is that this was caught in dev right now, before even merge to master, thanks to good PR checks. If not caught now it had another chance to be caught by torture in pre-release checks.

@Rdatatable Rdatatable deleted a comment from codecov bot Jul 17, 2019
@mattdowle mattdowle removed the WIP label Jul 17, 2019
@mattdowle mattdowle merged commit b4057af into master Jul 17, 2019
@mattdowle mattdowle deleted the cplx_gforce branch July 17, 2019 20:16
@mattdowle mattdowle mentioned this pull request Jul 17, 2019
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants