Warn when running out of memory instead of segfaulting #251
Conversation
sambrightman
commented
Sep 29, 2016
sambrightman
commented
- continue with the currently allocated memory
- warn continuously (maybe not ideal?)
|
In my testing this produces the same sort result, although in the low memory case it slightly differs for reads that compare equally (same chromosome, position and strand direction). Obviously this is due to them being split up and merged differently. Wonder if there is a way to make "equal" reads sort stably in this situation (fall-back to name?). |
|
It's probably worth adding a test in the |
|
Hi, thanks for looking into this. Sorting is unstable, and that's fine in my opinion, although it could mean minor reproducibility concerns. Fallbacks raise the question where to stop. (E.g. Picard includes flags and even some tags into comparison.) Continuous warnings can be avoided by introducing a flag, so that if it's set in case of failure, |
|
I think for coordinate sort Picard code is here. Reproducibility is useful for regression testing but a separate concern from this PR, so maybe take it up elsewhere. I think in the code further down there has to be an immediate exception because trying to grow |
sambrightman
force-pushed
the
sortsegfault
branch
from
415fd16 to
1005af2
Compare
* continue with the currently allocated memory * warn continuously (maybe not ideal?)
sambrightman
force-pushed
the
sortsegfault
branch
from
1005af2 to
0055d20
Compare
| } | ||
|
|
||
| void clear() { | ||
| _used = 0; | ||
| _n_reads = 0; | ||
| if (_low_memory) { | ||
| auto realloc_storage = cast(ubyte*)std.c.stdlib.realloc(read_storage, max_sz / 2); | ||
| if (realloc_storage !is null) { |
There was a problem hiding this comment.
Technically, this realloc can also fail (could reallocate via allocate/copy/free). I choose not to throw an exception here, but to keep printing errors. This is a situation in which we can proceed, but sub-optimally, and cannot automatically do anything to improve things. Spamming the user probably highlights the problem but results should still be correct.
| read_storage = cast(ubyte*)std.c.stdlib.realloc(read_storage, len); | ||
| auto realloc_storage = cast(ubyte*)std.c.stdlib.realloc(read_storage, len); | ||
| if (realloc_storage is null) { | ||
| throw new Exception("realloc failed: not enough memory for read"); |
There was a problem hiding this comment.
Since we are already trying to grow the storage, think there's nothing we can do here if it fails, so throw.
| throw new Exception("realloc failed: not enough memory for read"); | ||
| } else { | ||
| read_storage = realloc_storage; | ||
| max_sz = len; |
There was a problem hiding this comment.
max_sz was not being updated previously, so the storage would grow but not get used past max_sz in future fills.
|
Thanks, looks good to me. |
