use immediately invoked lambdas in size and range error checks #2255
Conversation
…ode in a cold path for better inlining
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
I'm not in for the The COLD_PATH stuff... Wow what a speedup. Looks worth it |
|
In totally fine with removing the no check macro. It was mostly just an idea |
There was a problem hiding this comment.
On the other hand I like STAN_NO_RANGE_AND_SIZE_CHECK. I believe we already have some functions in which significant amount of time is spent in checks. Also this is not much work to maintain. It is just an option for early return, without any special cases. Although we may be able to find a better name for this macro. Maybe STAN_RETURN_IF_NO_CHECKS?
|
What about STAN_NDEBUG ? |
Oh wait, why are we reinventing the wheel here why don't we just use NDEBUG?? |
|
NDEBUG is fine for me as well... |
It just seems more complicated. Like how is this exposed to the surface? The cold path stuff is for free to the user. I think this would definitely make sense for the O1/O2/O3 discussion. Like, turn off all nan/inf (the expensive ones) at O3 or something -- but that's just a different thing. |
…ro to decide to turn on or off range checks
|
It's exposed to the surface by the user defining |
|
Without a need for this outside of benchmarking Math, or without a plan to expose them at the interfaces, they're really not doing anything and I don't see why we'd add them. We can always add them once we've got a good reason. The idea doesn't go away if we don't put them in now. I wouldn't call this debug information. It is the intended behavior of the Math library to do these checks. The COLD_PATH stuff is different if I'm understanding correctly. Big performance difference and anyone using gcc gets it for free. |
|
While I do like the idea to be able to kill those checks for the sake of speed, I do concur here that this is actually a separate thing to sort out. These checks have triggered many debates - heated ones - and it‘s probably better to do that in a dedicated PR. (I am in favor for having the knob to turn this off, but still let‘s do one by one) |
|
I agree this should be a separate thing and should not be |
I'm not sure I'm following, what do you mean when you say we don't have a plan to expose these at the user interface? Or maybe a better question is, what does that look like to you? In my mindplace the user can just add
It does do something, which is that it makes the functions do nothing! Since this makes these functions no-ops the compiler completely ignores them
Yes and the std library treats it the same way where the intended behavior of
There's two things happening here.
|
Yeah, the use case makes sense. Dropping checks has come up before (I've heard @seantalts talk about this, and @bgoodri may have as well, though he can correct me). We could probably optionally turn off nan/infs/positive definite and bounds checks separately. It's a popular idea just needs talked to the interfaces, because that's where it will get used.
The interfaces mean how will people use it from cmdstan, cmdstanr, cmdstanpy, rstan, or pystan. Things like the threading on/off flags haven't been a walk in the park with how make and the precompiled headers work (though I think Rok has some sort of solution there now) even though there is a clear use case. rstanarm and brms and whatnot could probably use it too. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
imo I'd rather not turn these off since the turning off non/inf/ number bounds checks is usually associated with things like
Why wouldn't the user just add |
One fear I have for this is that some more advanced users may already have this flag and expect to still get the "normal" Stan behavior. Though that can be avoided by prominently displaying this in the release notes. I still think we should avoid NDEBUG. I am a bit less worried if this only concerns indexing bounds checks and not element-wise checks for nan/inf. For those I would be worried.
I dont think so, worst case the user needs to rebuild cmdstan after setting those. That is advised in general to anyone that touches CXXFLAGS directly. We have "quality-of-life" improvements when using For rstan/pystan I have no idea how/if they handle prebuilding main.o and CRTP and all that. I think we should not cause more issue for those tightly-coupled interfaces by suggesting users to try NDEBUG there. If someone is this concerned with speed they should be using cmdstan in the first place, imo. |
|
I still think adding an option to turn off checks is a good idea, but I think it should not use |
|
I removed the macro to remove checks right now, but we should have something like that in the future |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
Is this good to go? |
|
No. You have not addressed everything from my last review. |
|
Whups sorry missed that, removed the extra compiler attributes |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Summary
See issue #2249 for technical info. This uses immediately invoked lambdas in size and range error checks. This makes the actual code for the error checks smaller which should help the compiler when attempting to inline these.
This also adds some useful compiler attribute macros. One to call out in particular is
STAN_REMOVE_RANGE_AND_SIZE_CHECKSwhich is the user passes in at compile time will turn off all range and size checksTests
Refactor so no new tests
Side Effects
We should generally do this pattern in all the error checks so in a future PR would be good to do that
Release notes
Use immediately invoked lambdas in size and range error checks
Checklist
Math issue Use immedietly invoked lambdas to make error checking less expensive #2249
Copyright holder: Steve Bronder
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested