bool is_empty() for not writing to files - redo of 3081 #3082
Conversation
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
SteveBronder
left a comment
SteveBronder
left a comment
There was a problem hiding this comment.
@rok-cesnovar I approved this (though I wrote the original code) If this looks good to you then I think we can merge
|
I have been testing this over the weekend (with a change in cmdstan). Still seeing a perf regression :/ Will more details a bit later. |
|
Oh dang, can you send me the branch you are trying out? I think you just want to make this line to diagnostic_writers.emplace_back(
std::make_unique<std::fstream>(diagnostic_filename,
std::fstream::out, "#", true),That should turn off all of the diagnostics to know whether or not writing diagnostics is the issue. You can also do the same for the other writer for output. If that fixes the regression then the actual issue is writing to the stringstream before writing to disk which I think we can be looser about now that I'm thinking about it. I think at the time I assumed multiple threads could write to one |
|
The model used for testing: transformed data {
int N = 10000;
}
parameters {
vector[N] a;
}
model {
a ~ normal(0,1);
}Then various versions: Between each switch, I ran make stan-update, make clean-all, make build. Also set STANC3_VERSION=v2.x.y so the correct stanc3 binary is associated.
Elapsed Time: 8.892 seconds (Warm-up)
Elapsed Time: 8.296 seconds (Warm-up)
Elapsed Time: 8.752 seconds (Warm-up) |
|
Link to branch: https://github.com/stan-dev/cmdstan/tree/fix_diagnostic_writer So I guess I should have put it better, it looks better, but still not at the level of 2.27.0. Maybe that is fine and expected and due to the stream stuff? I would be fine with this amount of a perf. regression as long as we know where it comes from. |
|
It looks like it's faster than 2.27.1? Which would be before any of these changes |
|
Yeah, it is faster than 2.28.1. Maybe the rest of it is due to the use of streams and there is not much we can do besides that. |
|
Sorry am I not reading your posted times right?
Is the top one 2.27.1 and the bottom this branch? |
|
Ah, sorry I think I botched something. Let me run the benchmark again. I think the last number is wrong. Will run again to avoid any confusion... |
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
Closing this as I think I found a simpler solution in #3087 |
Submission Checklist
This is just a re-do of #3081 done with the master branch as base. The changes are all Steve's.
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Steve Bronder
Rok Češnovar (all I did was create the branch and open the PR though)
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: