Hi,
Sometimes I find adding assert() calls in my code very handy for various reasons: - failures in development mode on some complex code where exposing all the details to unit tests is sometimes hard and/or pointless - readability of the code
But I worry about the perf implications of these lines of code. I don't want these assertions to be used to track errors in production mode.
PHP7 introduced expectations which permit to have zero-cost assert() [1] Looking at the MW codebase we don't seem to use assert frequently (only 26 files [2] ).
Are there some discussions about this? Is assert() a good practice for the MW code base? If yes would it make sense to benefit from zero-cost assertions in WMF appservers?
Thanks!
[1] http://php.net/manual/en/function.assert.php#function.assert.expectations [2] https://codesearch.wmflabs.org/search/?q=%5Ctassert%5C(&i=nope&files...
Replying to myself: I just found some discussions here: https://lists.gt.net/wiki/wikitech/378676 I bet that the new assert features in PHP7 don't change the conclusions here, so please ignore my e-mail and sorry for the noise.
On Thu, Mar 15, 2018 at 2:42 PM, David Causse dcausse@wikimedia.org wrote:
Hi,
Sometimes I find adding assert() calls in my code very handy for various reasons:
- failures in development mode on some complex code where exposing all the
details to unit tests is sometimes hard and/or pointless
- readability of the code
But I worry about the perf implications of these lines of code. I don't want these assertions to be used to track errors in production mode.
PHP7 introduced expectations which permit to have zero-cost assert() [1] Looking at the MW codebase we don't seem to use assert frequently (only 26 files [2] ).
Are there some discussions about this? Is assert() a good practice for the MW code base? If yes would it make sense to benefit from zero-cost assertions in WMF appservers?
Thanks!
[1] http://php.net/manual/en/function.assert.php#function. assert.expectations [2] https://codesearch.wmflabs.org/search/?q=%5Ctassert%5C(&i=nope&files... php%24&repos=
Was the conclusion “don’t use assert()”? It’s not really that clear to me
(fwiw I've always felt a bit squiffy about assert()s in production code, because it’s easy to make a php config mistake and get errors happening all over the place)
On 15 Mar 2018, at 14:17, David Causse dcausse@wikimedia.org wrote:
Replying to myself: I just found some discussions here: https://lists.gt.net/wiki/wikitech/378676 I bet that the new assert features in PHP7 don't change the conclusions here, so please ignore my e-mail and sorry for the noise.
On Thu, Mar 15, 2018 at 2:42 PM, David Causse dcausse@wikimedia.org wrote:
Hi,
Sometimes I find adding assert() calls in my code very handy for various reasons:
- failures in development mode on some complex code where exposing all the
details to unit tests is sometimes hard and/or pointless
- readability of the code
But I worry about the perf implications of these lines of code. I don't want these assertions to be used to track errors in production mode.
PHP7 introduced expectations which permit to have zero-cost assert() [1] Looking at the MW codebase we don't seem to use assert frequently (only 26 files [2] ).
Are there some discussions about this? Is assert() a good practice for the MW code base? If yes would it make sense to benefit from zero-cost assertions in WMF appservers?
Thanks!
[1] http://php.net/manual/en/function.assert.php#function. assert.expectations [2] https://codesearch.wmflabs.org/search/?q=%5Ctassert%5C(&i=nope&files... php%24&repos=
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
The biggest take-away (for me) of the discussion is: Pros: - perf: zero-cost assertions Cons: - the benefits of zero-cost assertion is not worth the risk in a moving code-base like MW. The argument is that even in the case of assert being used properly (to expose strong expectations that cannot be unmet). Problems are too frequently found in production mode so benefiting from zero-cost assertions would be a risk in the sense that we would not be able to detect these errors.
On Thu, Mar 15, 2018 at 3:30 PM, Cormac Parle cparle@wikimedia.org wrote:
Was the conclusion “don’t use assert()”? It’s not really that clear to me
(fwiw I've always felt a bit squiffy about assert()s in production code, because it’s easy to make a php config mistake and get errors happening all over the place)
On 15 Mar 2018, at 14:17, David Causse dcausse@wikimedia.org wrote:
Replying to myself: I just found some discussions here: https://lists.gt.net/wiki/wikitech/378676 I bet that the new assert features in PHP7 don't change the conclusions here, so please ignore my e-mail and sorry for the noise.
On Thu, Mar 15, 2018 at 2:42 PM, David Causse dcausse@wikimedia.org
wrote:
Hi,
Sometimes I find adding assert() calls in my code very handy for various reasons:
- failures in development mode on some complex code where exposing all
the
details to unit tests is sometimes hard and/or pointless
- readability of the code
But I worry about the perf implications of these lines of code. I don't want these assertions to be used to track errors in production mode.
PHP7 introduced expectations which permit to have zero-cost assert() [1] Looking at the MW codebase we don't seem to use assert frequently (only
26
files [2] ).
Are there some discussions about this? Is assert() a good practice for the MW code base? If yes would it make sense to benefit from zero-cost assertions in WMF appservers?
Thanks!
[1] http://php.net/manual/en/function.assert.php#function. assert.expectations [2] https://codesearch.wmflabs.org/search/?q=%5Ctassert%5C(&
i=nope&files=
php%24&repos=
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Thu, Mar 15, 2018 at 10:39 AM, David Causse dcausse@wikimedia.org wrote:
The biggest take-away (for me) of the discussion is: Pros:
- perf: zero-cost assertions
Cons:
- the benefits of zero-cost assertion is not worth the risk in a moving
code-base like MW.
The biggest take-away for me from the discussions is that PHP5 assert()'s reliance on eval()-like operation is bad in a number of ways, and the weird behavior of issuing a warning and then exiting is weird.
PHP7's expectations seem like they started fixing those issues, although eval()-like use is still an option and exception-throwing seems to not be the default.
On Thu, Mar 15, 2018 at 3:57 PM, Brad Jorsch (Anomie) <bjorsch@wikimedia.org
wrote:
PHP7's expectations seem like they started fixing those issues, although eval()-like use is still an option and exception-throwing seems to not be the default.
indeed I must admit that it's rather concerning that assert( 'code' ) will try to evaluate the string arg and I completely overlooked this. I was focused on the perf arguments.
But what I understood from discussion is that even if PHP resolves these issues in the future it's unlikely that we will disable assertions in production mode for WMF appservers. So assert() will never be an option for performance hotspots.
Thanks for your feedback!
Hi!
PHP7's expectations seem like they started fixing those issues, although eval()-like use is still an option and exception-throwing seems to not be the default.
eval mode is deprecated in 7.2 which means that nobody should use it anymore. It's likely would not be deleted until the mythical 8.0 aka "version where the things can be broken" but for all practical purposes it should not be used by anybody now and is as dead as anything can be in 7.x.
On Thu, Mar 15, 2018 at 9:42 AM, David Causse dcausse@wikimedia.org wrote:
Looking at the MW codebase we don't seem to use assert frequently (only 26 files [2] ).
We generally use the wikimedia/assert library[3] instead. That's used a lot more often.[4] The README.md for that library includes some reasoning for using it over PHP's assert(), with links to past discussion.
[3]: https://packagist.org/packages/wikimedia/assert [4]: https://codesearch.wmflabs.org/search/?q=%5CtAssert%3A%3A&i=nope&fil...
Hi!
But I worry about the perf implications of these lines of code. I don't want these assertions to be used to track errors in production mode.
PHP7 introduced expectations which permit to have zero-cost assert() [1] Looking at the MW codebase we don't seem to use assert frequently (only 26 files [2] ).
According to https://phabricator.wikimedia.org/T172165 MW 1.31 will require PHP 7 or HHVM. However, I am not sure how the situation with zero-cost asserts are in HHVM and therefore in WMF production. Since WMF production is running HHVM now (this may change in the future but looks like that's the status for now) we should not rely on something HHVM may not support.
I would avoid assertions at all in performance-critical code paths. I.e. something that every microsecond counts. Example: code paths that were generating RDF dumps had to be hand-tuned to the point we rewrote RDF generation library, and there's still room for improvement there - and that's because each code path is hit 40M times when the dump for 40M entities is generated. However, most of the code in MediaWiki is not that intensely used and would be just fine with extra function call here and there.
We can also note that using Wikimedia\Assert precludes using zero cost asserts in the future, even if we migrate to PHP 7, and the assertion done this way can not be disabled or turned into zero-cost assertions. Depending on what function these assertions serve in the code, this may be a good thing or bad thing. I think in our case it is a good thing - most parts of the code are not CPU-critical to the tune that extra fcall would change things, and we have tons of runtime errors anyway and have a reasonable system around them to handle when they happen. So using Wikimedia\Assert is likely the way to go for most of MW code.
wikitech-l@lists.wikimedia.org