llvm dev - May 2017 - Enable STATISTIC all the time again?

On Wed, May 31, 2017 at 7:48 AM, Reid Kleckner <rnk at google.com> wrote:
> I think adding an option to cmake is fine, but it should definitely be off
> by default in release builds.
>
Why?

> If we make STATISTIC useful, then people will eventually put it on some
> hot path, and that will kill multi-threaded performance for users like
> ThinLTO, because multiple threads doing read-modify-write on the same
> memory is just slow.
>
With no offense meant: Can we please be driven by data?
This argument is "people will eventually do something silly and it will
make things slow for others". That is something that can be said about just
about anything in LLVM :).  This is why we have code review, etc.
If your concern is the atomic increments, then is there a reason to not
have it do the counting as a runtime option instead of a compile time one?
>
>
> On Wed, May 31, 2017 at 7:39 AM, Daniel Berlin via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Right now, the LLVM Statistic class does not increment values if we are
>> in non-debug mode, unless you define LLVM_ENABLE_STATS (which, sadly,
is
>> also not available from cmake).
>>
>> Before i go and add it to cmake, i ran numbers.
>>
>> I can't find a single testcase, large or small, where enabling
statistics
>> all the time isn't completely noise.
>>
>> It looks like it was disabled in march  2013.
>> https://reviews.llvm.org/D486
>>
>> The only discussion i can find in the same time period is around
fastisel
>> using a lot of per-instruction stats, and it causing some slow-down.
>>
>> However, I can't find any actual data/testcases in this discussion
at
>> all, and disabling stats globally is actually fairly annoying for
>> performance work.
>> :(
>>
>> Does anyone have any testcases where it is actually slow that i can
look
>> at?
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170531/0684ce1a/attachment-0001.html>

The original thread w.r.t. fast isel: http://lists.llvm.org/pi
permail/llvm-commits/Week-of-Mon-20130225/166934.html. Nadav cited 5% of
specific part of isel, but I don't know which benchmark. On my side, we
were testing build times for PNaCl
<https://codereview.chromium.org/20162004/diff/1/tools/nacl_perf_expectations/nacl_perf_expectations.json>
apps.
The setup would have been an LTO-like build, timing just the backend -- an
odd mix of getting optimized bitcode then running LLC under -O0. A minor
point, it also made the toolchain binaries a bit smaller (e.g., maybe 500KB
out of 12MB?). But, that's not a use case you'd need to worry about.


On Wed, May 31, 2017 at 11:17 AM, Daniel Berlin via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> On Wed, May 31, 2017 at 7:48 AM, Reid Kleckner <rnk at google.com>
wrote:
>
>> I think adding an option to cmake is fine, but it should definitely be
>> off by default in release builds.
>>
>
> Why?
>
>
>> If we make STATISTIC useful, then people will eventually put it on some
>> hot path, and that will kill multi-threaded performance for users like
>> ThinLTO, because multiple threads doing read-modify-write on the same
>> memory is just slow.
>>
>
> With no offense meant: Can we please be driven by data?
> This argument is "people will eventually do something silly and it
will
> make things slow for others". That is something that can be said about
just
> about anything in LLVM :).  This is why we have code review, etc.
> If your concern is the atomic increments, then is there a reason to not
> have it do the counting as a runtime option instead of a compile time one?
>
>>
>>
>
>> On Wed, May 31, 2017 at 7:39 AM, Daniel Berlin via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> Right now, the LLVM Statistic class does not increment values if we
are
>>> in non-debug mode, unless you define LLVM_ENABLE_STATS (which,
sadly, is
>>> also not available from cmake).
>>>
>>> Before i go and add it to cmake, i ran numbers.
>>>
>>> I can't find a single testcase, large or small, where enabling
>>> statistics all the time isn't completely noise.
>>>
>>> It looks like it was disabled in march  2013.
>>> https://reviews.llvm.org/D486
>>>
>>> The only discussion i can find in the same time period is around
>>> fastisel using a lot of per-instruction stats, and it causing some
>>> slow-down.
>>>
>>> However, I can't find any actual data/testcases in this
discussion at
>>> all, and disabling stats globally is actually fairly annoying for
>>> performance work.
>>> :(
>>>
>>> Does anyone have any testcases where it is actually slow that i can
look
>>> at?
>>>
>>>
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>>
>>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170531/7bc1cc49/attachment.html>

On 31 May 2017 at 08:17, Daniel Berlin via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> If your concern is the atomic increments, then is there a reason to not
have
> it do the counting as a runtime option instead of a compile time one?
Or increment into TLS variables and sum those at the end. Looking at a
statistic before the end of execution is dodgy territory anyway, but I
can't think of a case where it would be saner if other threads are
poking at it simultaneously.

Tim.

On Wed, May 31, 2017 at 10:44:01AM -0700, Tim Northover via llvm-dev
wrote:
> On 31 May 2017 at 08:17, Daniel Berlin via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > If your concern is the atomic increments, then is there a reason to
not have
> > it do the counting as a runtime option instead of a compile time one?
> 
> Or increment into TLS variables and sum those at the end. Looking at a
> statistic before the end of execution is dodgy territory anyway, but I
> can't think of a case where it would be saner if other threads are
> poking at it simultaneously.
Access to TLS variables can easily be more expensive than any atomics...

Joerg