[Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Frediano Ziglio]

> 
> > On 12 Feb 2018, at 13:34, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> >>> 
> >>> On 12 Feb 2018, at 11:22, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>> 
> >>>>> 
> >>>>> On 12 Feb 2018, at 09:21, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>>>> 
> >>>>>> 
> >>>>>> From: Christophe de Dinechin <dinechin at redhat.com>
> >>>>>> 
> >>>>>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> >>>>>> ---
> >>>>>> docs/spice_style.txt | 15 ++++++++++++---
> >>>>>> 1 file changed, 12 insertions(+), 3 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> >>>>>> index 13032df6..eef4880f 100644
> >>>>>> --- a/docs/spice_style.txt
> >>>>>> +++ b/docs/spice_style.txt
> >>>>>> @@ -99,10 +99,19 @@ FIXME and TODO
> >>>>>> 
> >>>>>> Comments that are prefixed with `FIXME` describe a bug that need to be
> >>>>>> fixed. Generally, it is not allowed to commit new code having `FIXME`
> >>>>>> comment. Committing  `FIXME` is allowed only for existing bugs.
> >>>>>> Comments
> >>>>>> that are prefixed with `TODO` describe further features, optimization
> >>>>>> or
> >>>>>> code improvements, and they are allowed to be committed along with the
> >>>>>> relevant code.
> >>>>>> 
> >>>>>> -ASSERT
> >>>>>> -------
> >>>>>> +Assertions
> >>>>>> +----------
> >>>>>> +
> >>>>>> +Use assertions liberally. Assertions help testing function arguments
> >>>>>> and
> >>>>>> function results validity. As a result, they make it easier to detect
> >>>>>> bugs.
> >>>>>> Also, by expressing the intent of the developer, they make the code
> >>>>>> easier
> >>>>>> to read.
> >>>>>> +
> >>>>> 
> >>>>> I come from the Windows world and I found here in our case the "Use
> >>>>> assertions
> >>>>> liberally" a bit stronger if they are always used.
> >>>> 
> >>>> I’m sorry, I can’t make sense of this sentence. I believe you mean that
> >>>> the
> >>>> recommendation is too strong (not stronger) because we never disable
> >>>> them?
> >>>> 
> >>> 
> >>> yes, "too strong”
> >> 
> >> OK. Now I understand what you mean.
> >> 
> >>> 
> >>>> Do you have any performance numbers on the extra cost of assertions?
> >>>> 
> >>> 
> >>> Does it matter?
> >> 
> >> Yes it does. When someone refrains from writing an assert because they are
> >> afraid of the extra cost, it generally mean they have no clue and switched
> >> to FUD mode. A typical assert on a modern CPU is a test and a branch, so
> >> we
> >> are talking nanoseconds.
> >> 
> > 
> > If the rule is "never care" that include any path in the code.
> 
> OK. “Liberally” IMO does not mean “don’t care”.
> 
> Liberally means “feel free to add them as you identify conditions that are
> worth checking”. But based on the discussion, I think we need to add things
> to pay attention to, namely:
> 
> - Avoid side effects
> - Don’t use assert as an alternative for error checking
> - Be mindful of timing
> 
> If we add that, does that address your concerns?
> 

Yes.

> > As a practical example take into account Quic code. In encode function
> > there are 4 spice_assert. encode is called for each pixel of the image
> > to million of times for a big image.
> 
> Well, I decided to resort to hard data to figure out how much this case
> really mattered.
> 
> With a reduced test case like this:
> 
> https://pastebin.com/inwJnAmd
> 
> Running 10000 iterations with assertions gives: “real	0m11.507s"
> Running without assertions gives: "real	0m0.128s"
> 
> So this is one case where assertions represent 100x the payload!!! Definitely
> worth looking into. If you ever want to use ENABLE_EXTRA_CHECKS anywhere,
> that would be a good place ;-)
> 

Ouch... was not expecting so bad either!

> Also, note that the loss is 10s for 10 billion pixels encoded, so that’s 1s
> per billion pixels, or 1s for 113 full 4K frames. At 60Hz, that suggests
> that the asserts could eat 50% of the CPU. Clearly, if I did not goof up in
> my evaluation, that needs some attention.
> 
> I filed https://spice-redmine.usersys.redhat.com/issues/62.
> 
> Finally, for the record, do not underestimate the compiler either. If you
> leave only the assertions, i.e run https://pastebin.com/DzPY7B1h, then you
> get a time of real	0m0.005s, and you will notice that the compiler just
> removed the assertions entirely. It also took me three attempts before
> getting arguments to “encode” where the compiler would not statically remove
> the assertions. My first experiments gave absolutely no effect of
> assertions, because assertions were just optimized away at compile-time.
> 
> 
> > Yes, the test is taking nanoseconds
> > but multiplied by millions turn it to milliseconds multiplied by the images
> > and frames and VMs in a machine they starts to count.
> 
> To me, the only way to decide is to offer hard evidence, as I did above.
> 
> Otherwise, it’s really speculation. In the example above, with variations in
> just the call arguments, I can either have assertions wiped out statically
> or having them count for 100x more than the body of the function…
> 
> 
> > These spice_assert(s) came probably from a long history path where
> > ASSERT/assert were used but not compiled in release code.
> > Other example is functions like ring_remove which for performance reasons
> > somebody decided to define inline but some with all the spice_assert in it
> > are enough big to take quite some bytes.
> 
> Again, what matters to me is to perform actual measurements. Someone measures
> with and without, we see what the bloat is, and we decide if it matters.
> Otherwise, just speculation. Granted, guided by intuition and experience,
> but still, it’s always a good idea to validate the best intuitions.
> 

Yes, make sense.

> 
> > 
> >> 
> >>> Depends on many thing, but the "use liberally" seems to
> >>> indicate that we never care about the costs.
> >> 
> >> No, it means that in general, we do not care about the cost of an assert.
> >> 
> >> Of course, don’t be stupid and write code like:
> >> 
> >> 	assert(recursive_fibonacci(50) > 0)
> >> 
> >> because then the assert might cost a little more than a few nanoseconds
> >> (about 36 seconds on my laptop).
> >> 
> >> But in reality, asserts are generally used to test very simple conditions.
> > 
> > In Windows people expect these checks to disappear in release so people
> > do more expensive checks.
> > This "small" setting change make a very big difference.
> 
> Again, I believe we need some actual measurement regarding for example code
> bloat and runtime overhead.
> 
> 
> > 
> >> Bad cases of an expensive assert in performance critical code are
> >> extremely
> >> rare. If this happens, I expect code review to catch it, and even so, I
> >> would not even consider the removal of an assert for that reason without
> >> hard numbers. Which is why I asked you about hard numbers above :-)
> >> 
> > 
> > But the style apply also to any future code so it catches also the addition
> > case, not only removal. And as code was moved from ASSERT to
> > spice_assert/g_assert
> > implicitly (maybe without people realizing) turned out in many additions.
> 
> I still do not deviate from my “liberally”. However, you convinced me that
> there are asserts in performance sensitive code in SPICE that require some
> love ;-)
> 
> > 
> >> Remember, I recently did some measurements to prove that a record entry
> >> would
> >> not be too expensive to be left in the code all the time. Assertions are
> >> generally even less expensive than a flight recorder entry.
> >> 
> >> If you want, what I could do is add a rule that in general, the expression
> >> inside an assert should make no function calls and have no side effects.
> >> That would have two benefits:
> >> 
> >> - By avoiding function calls, you restrict expressions to things that can
> >> be
> >> computed locally, which generally means a few CPU cycles at most
> > 
> > But is hard to enforce in case of inline functions.
> 
> No function call means inline or otherwise.
> 
> > 
> >> - By avoiding side effects, you guarantee that the assert has no impact on
> >> the behavior of the program.
> >> 
> > 
> > I think no side effects (beside timing) are mandatory for asserts.
> 
> What do you mean, “mandatory”? I can write “assert(*p++=0)”, the compiler
> will not complain. So it’s not the language nor the definition of “assert”
> that makes it mandatory.
> 

As I said coming from Windows and having debug with assertion compiled in and
release without you don't want debug and release to behave differently.
That was my "mandatory" and is also mentioned in all C documentation.

> So I think you are saying that it’s worth adding to the guidelines as being
> “mandatory”???
> 

But not hurt repeating.

> > 
> >> Also, for C++ code, consider C++G I.6, i.e. thinking about writing
> >> contract-like stuff, with “expect” for assertions that check
> >> pre-conditions,
> >> etc.
> >> 
> > 
> > I.6 also spoke about the debug/production behaviour.
> 
> Yes. The point was that it might be interesting to start thinking about
> contracts, i.e. maybe use different messages for preconditions,
> postconditions and internal assertions. Mostly for documentation purpose.
> 

Ok, so this was an extension for the future.

> > 
> >> 
> >>> 
> >>>> 
> >>>>> Kind of always using
> >>>>> address sanitizer compiled in even in production. Recently in
> >>>>> spice-server
> >>>>> we added code like:
> >>>>> 
> >>>>>  if (ENABLE_EXTRA_CHECKS) {
> >>>>>      spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> >>>>>      spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> >>>>>  }
> >>>>> 
> >>>>> the ENABLE_EXTRA_CHECKS is always true/false and is a compiled in
> >>>>> constant
> >>>>> and is true only if explicitly enabled so the compiler will strip the
> >>>>> entire
> >>>>> checks if not enabled (but still checking for syntax).
> >>>> 
> >>>> Yes. This kind of code is the obvious result of being a bit confused on
> >>>> the
> >>>> usage of assertions.
> >>>> 
> >>>> That specific example is somewhat ridiculous, by the way. The two tests
> >>>> are
> >>>> one machine instruction each, I doubt they are even noticeable on any
> >>>> performance benchmark.
> >>>> 
> >>> 
> >>> One machine instruction? In which architecture?
> >> 
> >> We are not going to play that game again, are we? You know exactly what I
> >> mean.
> >> 
> >> Any architecture I know of can do an integer compare for equality or >= in
> >> one machine instruction. That’s what I’m talking about.
> >> 
> >> Then obviously, depending on the surrounding context, you will also
> >> probably
> >> have a load (likely to be from hot cache, unless you are asserting for
> >> something that is not related to surrounding code), and a never-taken
> >> conditional branch, which the compiler is likely to mark statically as not
> >> taken because the assert macros nowadays have hints to that effect. Such a
> >> branch is effectively a no-op in most cases. So the effect on the
> >> surrounding code is complicated. In some cases, it may be null, e.g. if
> >> the
> >> value is already in a register, since the jump itself is going to be
> >> essentially a no-op.
> >> 
> > 
> > I think we can agree the time is negligible in the cold path case
> > and also in these cases code cost is not so high.
> 
> Yes. You also convinced me there are presently assertions in hot code.
> 
> > 
> >> 
> >>> Yes, this is not really hot path but I think there's a balance between
> >>> probability that the event happens, cost of not having the check and
> >>> costs having it. The boundaries are personal opinions.
> >> 
> >> No, the boundaries are not personal opinion, that is precisely my point.
> >> 
> > 
> > The fact that Windows setting are so different than Unix is IMO
> > a prove that these boundaries are opinions.
> 
> What I meant by “not personal” is that I won’t write in the guidelines:
> 
> “Choosing if an assert is too expensive is left to the personal opinion of
> the developer”.
> 
> On the other hand, I am perfectly willing to add something like:
> 
> “An assert that, under some specific benchmark, consumes a measurable amount
> of CPU time can be considered ‘hot’ and should use spice_hot_assert”
> 
> (where spice_hot_assert would be an assert that is only active under
> if(ENABLE_EXTRA_CHECKS))
> 

A 2 level assert can be helpful actually.

> But IMO, that’s still not a personal opinion, it’s a team guideline that I’d
> like to see as objective. Then the asserts in encode() are hot by that
> definition.
> 

Yes, should be measured and take into account the difference.
Still some member could say that 20% difference is too much or not.
Also we may argue on the use case where this 20% came.
I personally think also there could be opinions on how efficient is
a test (how often can happen or if is pure paranoia), testing if an
integer can safely contain 32 is quite madness, but some NULL pointer
checks can be paranoid too.

> 
> > 
> >> If the code in question is a code where you are carefully watching for
> >> load
> >> latencies, cache utilization, and where an additional load or jump will
> >> impact performance, then by all means, avoid asserts in that code. That is
> >> *never* the case in spice code that I am aware of.
> >> 
> >> Again, if that code is so performance-sensitive that you are pondering
> >> whether it’s OK, performance-wise, to add this expression to your code:
> >> 
> >> 	*p ? “Hello” : “World"
> >> 
> >> then OK, you have a point. I’m saying that because the expression above
> >> does
> >> exactly the same thing as the assert: a load, a test and a branch.
> >> 
> >> What I am arguing is that we are, by far, not in code that is anywhere
> >> close
> >> to this situation, especially not in the code that has the if
> >> (ENABLE_EXTRA_CHECKS). I regret to not have seen that when the patches
> >> were
> >> submitted.
> >> 
> >>> 
> >>>> ENABLE_EXTRA_CHECKS is also used for additional checks that are somewhat
> >>>> expensive at run-time, like in display_channel_finalize. OK, but then
> >>>> why
> >>>> not make that a run-time flag instead of a compile-time flag? Unless the
> >>>> additional test code makes your code larger by 50%, but I doubt this is
> >>>> the
> >>>> case here…
> >>>> 
> >>> 
> >>> Yes, as I said are opinions. And as I said ENABLE_EXTRA_CHECKS is a
> >>> compile
> >>> time constant true or false so has no costs at runtime.
> >> 
> >> Of course I know it’s a compile-time constant, thank you very much ;-)
> >> 
> >> What I am disputing is that using a compile time constant here matters at
> >> all. I am also disputing that it’s a good idea. It carries the wrong
> >> impression that either:
> >> 1) Performance is so sensitive that we can’t even afford an assert here,
> >> or
> >> 2) That the asserts fail for the moment, and are temporarily disabled
> >> (which
> >> is how I read that code initially)
> >> 
> >> So that code is not just ill-advised, it carries the wrong impression to
> >> someone reading it.
> >> 
> > 
> > Not for me.
> 
> Well, disagreeing on this one. This is ugly and confusing, if only because I
> don’t think that code is hot (that I have not measured yet, maybe I’m
> wrong).
> 

No, I don't consider that code hot, just the tests paranoid for release.

> > 
> >> I claim that it’s also totally useless. I’m willing to bet on it. If you
> >> can
> > 
> > Is not useless. Just not worth in production.
> 
> But the documentation for “ENABLE_EXTRA_CHECKS” documents "expensive checks”.
> How are the check in handle_msg_data “expensive”? Maybe Jonathon should
> answer that one, since he wrote that code.
> 

Don't blame Jonathon, was me, all my fault :-)

I paid my first 4mb expansion with months of work (well, was studying
at that time so having small jobs here and there).

> > 
> >> build a test case, no matter how contrived, where you can measure the
> >> performance impact on the spice server of the following patch using an
> >> external tool, and you can actually isolate that performance impact out of
> >> the overall measurement noise reliably, then I’m willing to pay a round of
> >> beers to the whole team next time we meet:
> >> 
> >> 
> >> diff --git a/server/stream-device.c b/server/stream-device.c
> >> index 4eaa959b..13ca3d1a 100644
> >> --- a/server/stream-device.c
> >> +++ b/server/stream-device.c
> >> @@ -168,10 +168,8 @@ handle_msg_format(StreamDevice *dev,
> >> SpiceCharDeviceInstance *sin)
> >> {
> >>     SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> >> 
> >> -    if (ENABLE_EXTRA_CHECKS) {
> >> -        spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> >> -        spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> >> -    }
> >> +    spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> >> +    spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> >> 
> >>     int n = sif->read(sin, dev->msg.buf + dev->msg_pos,
> >>     sizeof(StreamMsgFormat) - dev->msg_pos);
> >>     if (n < 0) {
> >> 
> >> 
> > 
> > as said the style does not affect a single piece of code
> > but present and future code.
> > 
> > The initial discussion started about the "liberally" definition
> > that for me are "we never care about costs”.
> 
> And that’s not what “liberally” means, I believe, see earlier.
> 
> 

Ok, maybe I got a too strong definition for this word.

> 
> 
> > 
> >> 
> >> 
> >>> 
> >>>>> 
> >>>>>> +Several form of assertion exist. SPICE does not use the language
> >>>>>> `assert`
> >>>>>> much, but instead relies on the following variants:
> >>>>>> +
> >>>>>> +- `spice_assert`, defined in `common/log.h`, is never disabled at
> >>>>>> run-time
> >>>>>> and prints an error if the assertion fails.
> >>>>>> +
> >>>>>> +- `g_assert` and other variants defined in glib such as
> >>>>>> `g_assert_null`,
> >>>>>> can
> >>>>>> be diabled by setting `G_DISABLE_ASSERT` (which is never done in
> >>>>>> practice
> >>>>>> for SPICE code), and emits a message through the g_assertion_message
> >>>>>> function and its variants.
> >>>>>> +
> >>>>> 
> >>>>> typo: "diabled" -> "disabled"
> >>>>> No, our code as far as I remember should NEVER be compiled and used
> >>>>> with
> >>>>> G_DISABLE_ASSERT enabled.
> >>>> 
> >>>> I mention in the text that we don’t disable them in practice.
> >>>> 
> >>>> But why should that NEVER be done? (in uppercase, no less)? If some
> >>>> distro
> >>>> has a policy of building with assertions disabled, that’s their choice.
> >>>> 
> >>> 
> >>> If you call g_assert to check runtime thing that can happen instead
> >>> of developer error and you remove the check at runtime this lead to
> >>> potential security issues so the NEVER that is we should ignore the
> >>> policy and build with assertions enabled even if the build policies
> >>> disable them. At this point is not their choice anymore.
> >> 
> >> Ah, that’s a good point. But that’s an obvious misuse of asserts. I hope
> >> we
> >> don’t rely on asserts for functionality in our code. If that’s the case,
> >> it
> >> must be fixed.
> >> 
> >> In any case, if that’s what we do, then the asserts are not a very good
> >> protection as far as security is concerned, since all they do is print
> >> something. It won’t abort your program, unlike the C library assert.
> >> 
> > 
> > I hope you are wrong here and spice_assert/g_assert does not print
> > and continue. If not we have a problem.
> 
> Well, to me,
> 
> #define spice_assert(x) G_STMT_START {                  \
>     if G_LIKELY(x) { } else {                           \
>         spice_error("assertion `%s' failed", #x);       \
>     }                                                   \
> } G_STMT_END
> 
> And then
> 
> #define spice_error(format, ...) G_STMT_START {                         \
>     spice_log(G_LOG_LEVEL_ERROR, SPICE_STRLOC, __FUNCTION__, "" format, ##
>     __VA_ARGS__); \
> } G_STMT_END
> 
> and then I don’t think spice_log aborts on “G_LOG_LEVEL_ERROR” by default.
> 
> Am I wrong there?
> 

In SPICE, as in Glib, ERROR is more "strong" than CRITICAL
(I don't know where this came, even in Solaris CRITICAL is more
strong like Linux, maybe an initial mistake).
By default CRITICAL is fatal (it calls abort) so ERROR is too.

> > I agree, asserts should not be used as needed runtime checks.
> 
> I’ll add that to the next writeup.
> 

Thanks.

> > 
> >> 
> >>> 
> >>>>> Or maybe I'm confused by G_DISABLE_CHECKS ?
> >>>>> OT: maybe we should check this in the code (I think better place is
> >>>>> common/log.c).
> >>>> 
> >>>> Check what? That G_DISABLE_ASSERT is not set?
> >>>> 
> >>> 
> >>> Yes
> >> 
> >> Well, if we do rely on them for security, then maybe we should abort on
> >> failure using the C library assert for that.
> >> 
> > 
> > Hope I'm wrong and I confuse other tests.
> > 
> >>> 
> >>>>> 
> >>>>>> +The guidelines for selecting one or the other are not very clear from
> >>>>>> the
> >>>>>> existing code. The `spice_assert` variant may be preferred for
> >>>>>> SPICE-only
> >>>>>> code, as it makes it clearer that this assertion is coming from SPICE.
> >>>>>> The
> >>>>>> `g_assert` and variants may be preferred for assertions related to the
> >>>>>> use
> >>>>>> of glib.
> >>>>>> 
> >>>>>> -Use it freely. ASSERT helps testing function arguments and function
> >>>>>> results
> >>>>>> validity.  ASSERT helps detecting bugs and improve code readability
> >>>>>> and
> >>>>>> stability.
> >>>>>> 
> >>>>>> sizeof
> >>>>>> ------
> >>>>> 
> >>> 
> > 

Frediano