llvm dev - Mar 2016 - RFC: Large, especially super-linear, compile time regressions are bugs.

Hi,
> There is a possibility that r259673 could play a role here.
>
> For the buildSchedGraph() method, there is the -dag-maps-huge-region that
> has the default value of 1000. When I commited the patch, I was expecting
> people to lower this value as needed and also suggested this, but this has
> not happened. 1000 is very high, basically "unlimited".
>
> It would be interesting to see what results you get with e.g. -mllvm
> -dag-maps-huge-region=50. Of course, since this is a trade-off between
> compile time and scheduler freedom, some care should be taken before
> lowering this in trunk.
Indeed we hit this internally, filed a PR:
https://llvm.org/bugs/show_bug.cgi?id=26940

As a general comment on this thread and as mentioned by Mehdi, we care
a lot about compile time and we're looking forward to contribute more
in this area in the following months; by collecting compile time
testcases into a testsuite and publicly tracking results on those we
should be able to start a RFC on a tradeoff policy.

-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc

LLVM has a wonderful policy regarding broken commits: we revert to green.
We ask that a test case be available within a reasonable time frame
(preferably before, but some exceptions can be made), but otherwise we
revert the offending patch, even if it contains nice features that people
want, and keep the tree green. This is an awesome policy.

I would like to suggest we adopt and follow the same policy for compile
time regressions that are large, and especially for ones that are
super-linear. As an example from the previous thread:

On Mon, Mar 14, 2016 at 12:14 PM Bruno Cardoso Lopes via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> > There is a possibility that r259673 could play a role here.
> >
> > For the buildSchedGraph() method, there is the -dag-maps-huge-region
that
> > has the default value of 1000. When I commited the patch, I was
expecting
> > people to lower this value as needed and also suggested this, but this
> has
> > not happened. 1000 is very high, basically "unlimited".
> >
> > It would be interesting to see what results you get with e.g. -mllvm
> > -dag-maps-huge-region=50. Of course, since this is a trade-off between
> > compile time and scheduler freedom, some care should be taken before
> > lowering this in trunk.
>
> Indeed we hit this internally, filed a PR:
> https://llvm.org/bugs/show_bug.cgi?id=26940

I think we should have rolled back r259673 as soon as the test case was
available.

Thoughts?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160331/cfde19c9/attachment.html>

Chandler Carruth via llvm-dev <llvm-dev at lists.llvm.org>
writes:
> LLVM has a wonderful policy regarding broken commits: we revert to green.
> We ask that a test case be available within a reasonable time frame
> (preferably before, but some exceptions can be made), but otherwise we
> revert the offending patch, even if it contains nice features that people
> want, and keep the tree green. This is an awesome policy.
>
> I would like to suggest we adopt and follow the same policy for compile
> time regressions that are large, and especially for ones that are
> super-linear. As an example from the previous thread:
>
> On Mon, Mar 14, 2016 at 12:14 PM Bruno Cardoso Lopes via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> > There is a possibility that r259673 could play a role here.
>> >
>> > For the buildSchedGraph() method, there is the
-dag-maps-huge-region that
>> > has the default value of 1000. When I commited the patch, I was
expecting
>> > people to lower this value as needed and also suggested this, but
this
>> has
>> > not happened. 1000 is very high, basically "unlimited".
>> >
>> > It would be interesting to see what results you get with e.g.
-mllvm
>> > -dag-maps-huge-region=50. Of course, since this is a trade-off
between
>> > compile time and scheduler freedom, some care should be taken
before
>> > lowering this in trunk.
>>
>> Indeed we hit this internally, filed a PR:
>> https://llvm.org/bugs/show_bug.cgi?id=26940
>
>
> I think we should have rolled back r259673 as soon as the test case was
> available.
>
> Thoughts?
+1. Reverting is easy when a commit is fresh, but gets rapidly more
difficult as other changes (related or not) come after it, whereas
re-applying a commit later is usually straightforward.

Keeping the top of tree compiler in good shape improves everyone's
lives.

On Thu, Mar 31, 2016 at 1:28 PM, Chandler Carruth via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> LLVM has a wonderful policy regarding broken commits: we revert to green.
We
> ask that a test case be available within a reasonable time frame
(preferably
> before, but some exceptions can be made), but otherwise we revert the
> offending patch, even if it contains nice features that people want, and
> keep the tree green. This is an awesome policy.
>
> I would like to suggest we adopt and follow the same policy for compile
time
> regressions that are large, and especially for ones that are super-linear.
> As an example from the previous thread:
+1
> On Mon, Mar 14, 2016 at 12:14 PM Bruno Cardoso Lopes via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>>
>> > There is a possibility that r259673 could play a role here.
>> >
>> > For the buildSchedGraph() method, there is the
-dag-maps-huge-region
>> > that
>> > has the default value of 1000. When I commited the patch, I was
>> > expecting
>> > people to lower this value as needed and also suggested this, but
this
>> > has
>> > not happened. 1000 is very high, basically "unlimited".
>> >
>> > It would be interesting to see what results you get with e.g.
-mllvm
>> > -dag-maps-huge-region=50. Of course, since this is a trade-off
between
>> > compile time and scheduler freedom, some care should be taken
before
>> > lowering this in trunk.
>>
>> Indeed we hit this internally, filed a PR:
>> https://llvm.org/bugs/show_bug.cgi?id=26940
>
>
> I think we should have rolled back r259673 as soon as the test case was
> available.
I agree, but since we didn't have a policy about it, I was kind of
unsure on what to do about it. Glad you begin this discussion :-)
> Thoughts?
Ideally it would be good to have more compile time sensitive
benchmarks on the test-suite to detect those. We're are working on
collecting what we have internally and upstream to help track the
results in a public way.

-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc

Hi,

TLDR: I totally support considering compile time regression as bug.

I'm glad you bring this topic. Also it is worth pointing at this recent
thread: http://lists.llvm.org/pipermail/llvm-dev/2016-March/096488.html
And also this blog post comparing the evolution of clang and gcc on this aspect:
http://hubicka.blogspot.nl/2016/03/building-libreoffice-with-gcc-6-and-lto.html

I will repeat myself here, since we also noticed internally that compile time
was slowly degrading with time. Bruno and Chris are working on some
infrastructure and tooling to help tracking closely compile time regressions.

We had this conversation internally about the tradeoff between compile-time and
runtime performance, and I planned to bring-up the topic on the list in the
coming months, but was waiting for more tooling to be ready.
Apparently in the past (years/decade ago?) the project was very conservative on
adding any optimizations that would impact compile time, however there is no
explicit policy (that I know of) to address this tradeoff.
The closest I could find would be what Chandler wrote in:
http://reviews.llvm.org/D12826 <http://reviews.llvm.org/D12826> ; for
instance for O2 he stated that "if an optimization increases compile time
by 5% or increases code size by 5% for a particular benchmark, that benchmark
should also be one which sees a 5% runtime improvement".

My hope is that with better tooling for tracking compile time in the future,
we'll reach a state where we'll be able to consider "breaking"
the compile-time regression test as important as breaking any test: i.e. the
offending commit should be reverted unless it has been shown to significantly
(hand wavy...) improve the runtime performance.

Since you raise the discussion now, I take the opportunity to push on the
"more aggressive" side: I think the policy should be a balance between
the improvement the commit brings compared to the compile time slow down.
Something along the line as what you wrote in my quote above.
You are referring to "large" compile time regressions (aside: what is
"large"?), while Bruno has graphs that shows that the compile time
regressions are mostly a lot of 1-3% regressions in general, spread over tens of
commits.
Also (and this where we need better tooling) *unexpected* compile-time slow down
are what makes me worried: i.e. the author of the commit adds something but
didn't expect the compile time to be "significantly" impacted.
This is motivated by Bruno/Chris data.
Tracking this more closely may also help to triage thing between O2 and O3 when
a commit introduces a compile time slow-down but also brings significant enough
runtime improvements.

-- 
Mehdi




> On Mar 31, 2016, at 1:28 PM, Chandler Carruth via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> LLVM has a wonderful policy regarding broken commits: we revert to green.
We ask that a test case be available within a reasonable time frame (preferably
before, but some exceptions can be made), but otherwise we revert the offending
patch, even if it contains nice features that people want, and keep the tree
green. This is an awesome policy.
> 
> I would like to suggest we adopt and follow the same policy for compile
time regressions that are large, and especially for ones that are super-linear.
As an example from the previous thread:
> 
> On Mon, Mar 14, 2016 at 12:14 PM Bruno Cardoso Lopes via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
> > There is a possibility that r259673 could play a role here.
> >
> > For the buildSchedGraph() method, there is the -dag-maps-huge-region
that
> > has the default value of 1000. When I commited the patch, I was
expecting
> > people to lower this value as needed and also suggested this, but this
has
> > not happened. 1000 is very high, basically "unlimited".
> >
> > It would be interesting to see what results you get with e.g. -mllvm
> > -dag-maps-huge-region=50. Of course, since this is a trade-off between
> > compile time and scheduler freedom, some care should be taken before
> > lowering this in trunk.
> 
> Indeed we hit this internally, filed a PR:
> https://llvm.org/bugs/show_bug.cgi?id=26940
<https://llvm.org/bugs/show_bug.cgi?id=26940>
> 
> I think we should have rolled back r259673 as soon as the test case was
available.
> 
> Thoughts?
> _______________________________________________
> 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/20160331/94af3e0d/attachment.html>