Changed Cells, CellLookup, MCells, and MCellsLookup to std::vector(...) #200
Conversation
chrisatang
requested review from
dlaydon,
gnedjati,
matt-gretton-dann,
NeilFerguson and
Z80coder
as code owners
| std::vector<cell> Cells(P.NC); // Cells.at(i) is the i'th cell | ||
| std::vector<cell*> CellLookup(P.NCP); // CellLookup[i] is a pointer to the i'th populated cell | ||
| std::vector<microcell> Mcells(P.NMC); | ||
| std::vector<microcell*> McellLookup(P.NMCP); |
There was a problem hiding this comment.
You are trying to initialise these at program start up time when P.NC is 0 - so this will fail - I think need to have something like:
std::vector<cell> Cells;
// Whereever the calloc was
Cells.resize(P.NC);etc.
| @@ -2567,25 +2568,25 @@ void InitModel(int run) // passing run number so we can save run number in the i | |||
| { | |||
| for (i = tn; i < P.NC; i+=P.NumThreads) | |||
| { | |||
| if ((Cells[i].tot_treat != 0) || (Cells[i].tot_vacc != 0) || (Cells[i].S != Cells[i].n) || (Cells[i].D > 0) || (Cells[i].R > 0)) | |||
| if ((Cells.at(i).tot_treat != 0) || (Cells.at(i).tot_vacc != 0) || (Cells.at(i).S != Cells.at(i).n) || (Cells.at(i).D > 0) || (Cells.at(i).R > 0)) | |||
There was a problem hiding this comment.
std::vector::at() will throw an exception on a bad access. Not a bad thing per se - however, the code isn't exception aware and I think there will be problems with exceptions crossing thread boundaries.
There are two solutions to this:
The easy one: Just use [] access everywhere. (Why do you use it in some places and not others?).
The harder one, put try/catch blocks everywhere that is appropriate.
There was a problem hiding this comment.
Just to expand on the uncaught exceptions issue flagged here, the OpenMP specification mandates that an exception thrown within the omp parallel for region must cause the thread to resume within the same region (ie. no uncaught exceptions within omp parallel for loops).
I second the suggestion to prefer the use of operator[], rather than wrapping inner loops in try catch blocks, since I don't think there's much that can be done about an invalid index in either case, plus a segmentation fault raised by an invalid index is more obviously wrong.
| @@ -4101,9 +4102,9 @@ void SaveSnapshot(void) | |||
| fprintf(stderr, "## %i\n", i++); | |||
| zfwrite_big((void*)Households, sizeof(household), (size_t)P.NH, dat); | |||
| fprintf(stderr, "## %i\n", i++); | |||
| zfwrite_big((void*)Cells, sizeof(cell), (size_t)P.NC, dat); | |||
| zfwrite_big((void*)&Cells.front(), sizeof(cell), (size_t)P.NC, dat); | |||
There was a problem hiding this comment.
| zfwrite_big((void*)&Cells.front(), sizeof(cell), (size_t)P.NC, dat); | |
| zfwrite_big((void*)Cells.data(), sizeof(cell), (size_t)P.NC, dat); |
Calling front results in undefined behaviour in the case where Cells is empty. C++11 introduced std::vector<T,Allocator>::data, which will simply return a null pointer in the case of an empty vector. We should use that here.
| @@ -2567,25 +2568,25 @@ void InitModel(int run) // passing run number so we can save run number in the i | |||
| { | |||
| for (i = tn; i < P.NC; i+=P.NumThreads) | |||
| { | |||
| if ((Cells[i].tot_treat != 0) || (Cells[i].tot_vacc != 0) || (Cells[i].S != Cells[i].n) || (Cells[i].D > 0) || (Cells[i].R > 0)) | |||
| if ((Cells.at(i).tot_treat != 0) || (Cells.at(i).tot_vacc != 0) || (Cells.at(i).S != Cells.at(i).n) || (Cells.at(i).D > 0) || (Cells.at(i).R > 0)) | |||
There was a problem hiding this comment.
Just to expand on the uncaught exceptions issue flagged here, the OpenMP specification mandates that an exception thrown within the omp parallel for region must cause the thread to resume within the same region (ie. no uncaught exceptions within omp parallel for loops).
I second the suggestion to prefer the use of operator[], rather than wrapping inner loops in try catch blocks, since I don't think there's much that can be done about an invalid index in either case, plus a segmentation fault raised by an invalid index is more obviously wrong.
Changed the Cells, CellLookup, MCells, and MCellsLookup from dynamic calloc(...) to std::vector(...), which is a start to resolving any possible memory issues the current code might have.