Re: [PATCH] btrfs: do not evaluate the expression with !CONFIG_BTRFS_ASSERT

[Josef Bacik <josef@toxicpanda.com>]

On 7/27/20 12:55 PM, David Sterba wrote:
> On Fri, Jul 24, 2020 at 12:41:47PM -0400, Josef Bacik wrote:
>> While investigating a performance issue I noticed that turning off
>> CONFIG_BTRFS_ASSERT had no effect in what I was seeing in perf,
>> specifically check_setget_bounds() was around 5% for this workload.
> 
> Can you please share the perf profile and .config?  I find it hard to
> believe that check_setget_bounds would be taking 5% overall. Also you
> said that this was with integrity-checker compiled in so this kind of
> invalidates any performance claims.
> 

How?  It was a straight A/B test, do you think I'm making this up?

> I've been watching perf top for various debugging and release builds for
> some time and this one makes it to top 5 but never #1 or #2.
> 
> The function compiles to a few instructions and the hot path is
> correctly predicted by compiler, so I'm really curious what's so special
> about the workload that it needs to call it in 1/20th of overall time.
> 
>> Upon investigation I realized that I made a mistake when I added
>> ASSERT(), I would still evaluate the expression, but simply ignore the
>> result.
> 
> Vast majority of the assert expressions are simple expressions without
> side effects, but compiler still generates the code. In most cases it's
> a few no-op movs, so this leaves the impact on the function calls.
> 
> Making the assert a true no-op saves some asm code and gains some
> performance, but I don't want to remove the check_setget_bounds calls as
> it's another line of defence against random in-memory corruptions.
> 
> Given that it's called deep inside many functions, it would be
> impractical to add checking of each call. Instead, we can set a bit and
> do a delayed abort in case it's found. I have that as a prototype and
> will post it later.

Then make it configurable, because with ECC memory the performance overhead 
isn't worth it.

> 
>> This is useless, and has a marked impact on performance.  This
>> microbenchmark is the watered down version of an application that is
>> experiencing performance issues, and does renames and creates over and
>> over again.  Doing these operations 200k times without this patch takes
>> 13 seconds on my machine.  With this patch it takes 7 seconds.
> 
> Do you have that as a script?
> 

Yeah, you can find it here.  It was written by somebody internally to illustrate 
an issue they're seeing with their application.

https://paste.centos.org/view/f01126bf

Thanks,

Josef