I’ve been wanting to write this down for a while. Just finding time and enough clarity now to do so.
tldr: my bad. We fixed it and released v2.17.1. We have measures in place to prevent an issue like this one.
What happened
We’ve had an open issue, stan-dev/math#460, to replace const char*
with std::string
.
It was assigned to me and I had some time, so I submitted a pull request, stan-dev/math#584, fixing the issue. It passed all tests in math. Just to be clear, this didn’t introduce a bug where behavior was incorrect. It slowed everything down. The pull request was reviewed and merged.
The problem was that I should have checked the performance of this change before submitting the pull request. Unfortunately, our unit tests for math don’t cover performance. I should have marked on the pull request that there was a possible performance hit. I don’t recall if I thought about that at the time (it was a large number of files changed at once and I think I was really happy that I got every instance correct; this might have been my 3rd attempt at fixing this issue).
Typically, I had been monitoring the performance tests in the stan-dev/stan repo to make sure large performance changes don’t go unnoticed. Recently before that pull request, there were changes to the math library that legitimately changed the performance and this coincided with less involvement from me on the continuous integration front. And to simplify the process, changes to the math library were being automatically merged into stan without a human (me) involved looking at the output. Anyway, I should have been paying attention to it, but it slipped. It was released with v2.17.0.
Thanks to this post by @jjramsey, we identified the cause of the slow down. @seantalts and @bgoodri stepped up and fixed the merge conflicts in git and we were able to patch both develop
and release a v2.17.1.
By the way, it’s still a good idea to change the code to make it cleaner. Hopefully static std::string
will have the same performance as const char*
.
Why it won’t happen (in the same way) again
Jenkins is now set up to notify if the performance changes in stan-dev/stan by a large amount. Thanks, @seantalts, for setting that up! (I had tried for a while, but never gotten it to work, which was why I was monitoring it by hand.)
What exactly is this testing? It’s running a (poorly coded) logistic regression that’s run 100 times with the same seed and records the timing (roughly 1.3 sec per run). It checks to see that:
- the last draw matches hard-coded values
- each run produces the exact same last draw
It’s run on the same machine every day and on every stan-dev/stan change (including updates to the math submodule). Given that it runs on the same machine with the same compiler, cpu, etc. what this tells us is that things run exactly the same between changes to the code (or it doesn’t and we should know about it), that we haven’t introduced randomness that destroys reproducibility, and a rough estimate of performance. It’s not a perfect test, but it helps with making sure we haven’t slipped at those things. Like a lot of things in Stan, it started out as a simple way to test timing, but then it evolved and we tacked on checking output to make sure things don’t change because we didn’t have an end-to-end test for that.
Things to think about
Here are just some thoughts and perhaps others can chime in and contribute some more tests:
- we don’t have any automated performance tests at the stan-dev/math level; we should add some
- the current tests are hard to update; it’d be nice if they were easier to update
- perhaps we should bring back the BUGS tests; for a long time, we ran all the BUGS examples and used their estimates of mean as a way to verify that our code didn’t break. We also used it as a crude performance test. When it was proposed to get rid of it (due to false positives), the current performance test was created so that we didn’t completely lose all end-to-end testing
- we had a thread about gold master testing, but I think the conclusion was that isn’t what we’re after. It’d be nice to introduce some more automated end-to-end tests to prevent performance bugs like this
Anyway, I just wanted to recap. I created the pull request that slowed everything down. I really should have mentioned the potential performance issue (even without tests). And I should have tracked this one through the whole process. But thankfully, we fixed it, and the fix has been released. More importantly, something that affects performance like this one won’t stay in develop
long thanks to Jenkins.