amount of camelCase refactoring causing some downstream overhead

Hi there,

At the end of last week we saw a number of commits go in that were camelCasing batches of MCStreamer::Emit* and AsmPrinter::Emit* functions.

For example:
- rG549b436beb41
- rGa55daa146166
- rG0bc77a0f0d16

Unfortunately all these individual commits trigger the same merge conflicts over and over again with our downstream repo, which takes us some manual intervention every time.

I understand uniformity is a nice to have, but:
1 - is it worth it to do this work right now? I can remember the casing debate a few months back, which seems unrelated to this work which seems manual, but I'm unsure of the outcome.
2 - If this work should be done, it would be nice if all of the work is done in one batch, to save us some of the downstream overhead.

Thanks
/Ties

My usual take on this would be that it’s within the LLVM project norms to fix up naming on a case by case basis (independent of the recent discussion you mentioned) - especially if different subsets of a single interface/group of related interfaces have become more visibly inconsistent.

My understanding is that LLVM's general policy is to not let
downstream slow down upstream development. The C++ API explicitly
unstable.

Note that we are even considering renaming variables globally:
https://lists.llvm.org/pipermail/llvm-dev/2019-September/134921.html

Michael

During that variable renaming debate, there was a discussion about discussion about doing things all at once, piecemeal or not at all. An issue that wasn't really resolved I think. I had the impression that the efforts fizzled out a bit, and I thought this renaming was maybe related to that, but I'm neutral on if we should do variable renaming.

All I'm asking as a kindness if we could be kind on poor downstream maintainers not on the issue of variable renaming at large, but on the micro level of not pushing 5/6 patches of this kind covering closely related functionality in two days but collating them in 1. I don't think that would slow down development, and I wanted to highlight the issue, as people might not be aware that they could save some pain in a simple way. Especially if indeed there is going to be a big renaming push and this would be a continuous thing.

Cheers,
/Ties

As others have said, our long standing policy has been that downstream projects must fend for themselves. We're certainly not going to reverse that policy without careful discussion of the tradeoffs.

I'm personally of the opinion that there could be a middle ground which allows upstream to move quickly while reducing headache for downstream projects. Given I wear both hats, I know I'd certainly appreciate such a state. However, it's important to state that such decisions would need to be carefully considered and would require some very careful drafting of proposal to balance the competing interests at hand.

If anyone is curious, I'm happy to share some ideas offline on what starting points might be, but I have neither the time nor the interest to drive such a conversion on list.

Philip

I don’t think anyone is arguing to change longstanding policy. From a downstream perspective many small renaming changes do increase overhead for us.

One thing that happens to downstream projects is that they support more than one LLVM version, we (JuliaLang) currently try to support latest stable + master.

So for me the question is more, are renaming changes worth downstream projects not being able to test and provide feedback to upstream? One way of mitigating that is to consciously schedule them just before a release and do them all in short succession.

-V

Valentin,

You are proposing to change existing policy. Current policy is that we don’t consider downstream at all. Your proposal may seem reasonable - it may even be reasonable - but it is definitely a change from historical practice and must be considered as such.

Philip

I want to mention a few points:

* These refactoring commits are made within the [20200213,20200215] time frame.
   It started on Thursday afternoon (Pacific Time). For many users, it is
   "oh, it started on Friday and ended in the weekend".
   They unlikely cause "over and over again" conflicts.
* MCStreamer is usually not inherited.
   AsmPrinter can be inherited by a downstream target (think lib/Target/$target).
   However, for most downstream users, they should not observe anything.
   The changes just appear to be gigantic. Its impact is actually much
   smaller than an interface change of DIBuilder::createGlobalVariableExpression,
   MaybeAlign, IRBuilderBase::CreateMemSet, or deleted implicit conversion
   from StringRef to std::string.
* Before the refactoring, AsmPrinter and MCStreamer were in a very
   unpleasant inconsistent status: many use Emit* while some use emit*.
   It was a pain to think "I probably need to use the uncommon Emit* because
   AsmPrinter/MCStreamer have a different convention".

Hi Philip,

While it’s true we don’t I think Valentin is reasonable in saying “hey, when people do this let’s try to combine them if it makes sense”. It’s just being polite to everyone, especially if it doesn’t risk the patches or upstream stability. I don’t think there’s a policy change being proposed, just a “hey, let’s see what we can do in the future”.

-eric

Hi Philip,

While it’s true we don’t I think Valentin is reasonable in saying “hey, when people do this let’s try to combine them if it makes sense”. It’s just being polite to everyone, especially if it doesn’t risk the patches or upstream stability. I don’t think there’s a policy change being proposed, just a “hey, let’s see what we can do in the future”.

I think the somewhat unspoken change in LLVM social conventions (& somewhat policy, I think it’s written down in some places) is the “keep patches as small as practically possible” - grouping unrelated renamings would be something I’d usually (without concern for downstream consumers) push back against for all the usual reasons: easier to review, easier to revert strategically if something goes wrong, etc.

What I’m not clear on is why one big rename patch is easier for a downstream consumer than two smaller renames - I haven’t fully understood the nature of this particular downstream consumer’s approach makes this interesting.

  • Dave

Hi Philip,

While it’s true we don’t I think Valentin is reasonable in saying “hey, when people do this let’s try to combine them if it makes sense”. It’s just being polite to everyone, especially if it doesn’t risk the patches or upstream stability. I don’t think there’s a policy change being proposed, just a “hey, let’s see what we can do in the future”.

I think the somewhat unspoken change in LLVM social conventions (& somewhat policy, I think it’s written down in some places) is the “keep patches as small as practically possible” - grouping unrelated renamings would be something I’d usually (without concern for downstream consumers) push back against for all the usual reasons: easier to review, easier to revert strategically if something goes wrong, etc.

What I’m not clear on is why one big rename patch is easier for a downstream consumer than two smaller renames - I haven’t fully understood the nature of this particular downstream consumer’s approach makes this interesting.

Mostly the range of “broken” revisions as an example. That said, I also very much want it to make sense rather than something else. :slight_smile: I agree with everything you’ve said anyhow.

-eric

I think the somewhat unspoken change in LLVM social conventions (& somewhat policy, I think it's written down in some places) is the "keep patches as small as practically possible" - grouping unrelated renamings would be something I'd usually (without concern for downstream consumers) push back against for all the usual reasons: easier to review, easier to revert strategically if something goes wrong, etc.

What I'm not clear on is why one big rename patch is easier for a downstream consumer than two smaller renames - I haven't fully understood the nature of this particular downstream consumer's approach makes this interesting.

The question that really matters is whether the rename is fully
automatic or not. If the rename is done by an automatic script (e.g.
clang-tidy based), then the same script can be run "downstream" ahead
of merges to avoid virtually all of the conflicts. And if the rename
is performed by an automatic script, it seems like it'd actually be
simpler for everybody to do it in one big patch, or perhaps one C++
namespace at a time.

The biggest remaining problem in this scenario is with *users* of LLVM
that call into the C++ API and aim to maintain source-compatibility
against multiple versions of LLVM. I currently cannot see how that
would work.

Full disclaimer about where I'm coming from: We (AMD's graphics
compiler) are affected by the rename on two counts:

1) In llvm-project itself, we're upstream (AMDGPU backend), but we do
have fairly long-lived internal branches for development against
unreleased hardware that are still kept up-to-date with automatic
merges multiple times per day.

2) Outside of llvm-project, the compiler frontend (llpc, which is also
open-source) also tracks llvm-project master closely and calls into
the C++ interface, so would also be affected.

(We currently don't attempt to maintain compatibility with multiple
versions of LLVM, though it's something that I'd like us to do at some
point.)

Cheers,
Nicolai

On behalf of my colleagues who do the actual merge work
(all of which I get to review, so I do see it all too),
let me say I feel the pain of the downstream projects.

I wouldn't change upstream policy, although in this particular
case (see details below) it certainly looked like the patches
were renaming subsets of APIs from *the same classes* in several
rounds, rather than all at once. That feels a bit unfriendly.

More inline.

From: llvm-dev <llvm-dev-bounces@lists.llvm.org> On Behalf Of Nicolai
Hähnle via llvm-dev
Sent: Tuesday, February 18, 2020 10:08 PM
To: David Blaikie <dblaikie@gmail.com>
Cc: llvm-dev <llvm-dev@lists.llvm.org>; Valentin Churavy
<v.churavy@gmail.com>
Subject: Re: [llvm-dev] amount of camelCase refactoring causing some
downstream overhead

> I think the somewhat unspoken change in LLVM social conventions (&
somewhat policy, I think it's written down in some places) is the "keep
patches as small as practically possible" - grouping unrelated renamings
would be something I'd usually (without concern for downstream consumers)
push back against for all the usual reasons: easier to review, easier to
revert strategically if something goes wrong, etc.

Right, except mechanical formatting/renaming patches fall outside that
convention. You don't clang-format a file in 3 rounds, you do it all
at once and get it over with. The set of renamings here were not
"unrelated" to my eye, it was renaming a big pile of nonconforming APIs
(all in MC, I think) in multiple rounds, that looked like they all affected
essentially the same set of files.
Doing those in multiple patches was about as bad as a commit-revert-commit
sequence, from a downstream merge POV. The latter is unavoidable, but the
former IS avoidable.

>
> What I'm not clear on is why one big rename patch is easier for a
downstream consumer than two smaller renames - I haven't fully understood
the nature of this particular downstream consumer's approach makes this
interesting.

If you have (as I do) nonzero amounts of downstream code calling those
APIs, then each change is at least a build fail if not an actual merge
conflict. Fixing these in N rounds costs more than fixing all in one go;
you're doing 1/N as much editing each time but still doing N builds to
verify before you can commit the fixup.
Each conflict/failure is an interruption to the auto-merge and increases
how far behind it gets, until we have a period of no-problem merges and
can catch up again. We don't like falling behind; we'd rather keep up
so problems that need fixing upstream can be done in a timely way.

--paulr

Eric,

I disagree. Strongly. I see the very fact we’re engaging in the discussion of “just being polite” here as normalizing a proposed change in policy which has potentially profound negative consequences for the project long term. I do not want upstream developers “trying to be polite” if that delays otherwise worthwhile work. The current policy is “downstream is on their own”. There was nothing even remotely unreasonable done in the patch series which triggered this discussion and I don’t want any upstream contributor coming to believe there was.

Again, I’m open to carefully weighted proposals to change current policy. I also have a downstream repo which is kept up to date and I understand the pain point being raised. I just want to be very careful to distinguish between existing status, and any proposed changes. I want the proposed changes to be carefully weighed before being put into effect.

Philip

Hi Philip,

You and I are going to disagree then. Strongly and strenuously every time. As someone who has reiterated the same policy multiple times I don’t see anything around “try to make it easy on downstream if you can without making it harder for upstream” that contradicts any policy or even tries to set anything. There is no policy under discussion, just a “hey, see if you can do this in a friendly way” next time is just fine. If you and I can’t agree on that then I really see no point in discussing anything further with you.

-eric

Hi Philip,

I think you might be reading more into the suggestion/discussion than is actually there.

  • I do not want upstream developers “trying to be polite” if that delays otherwise worthwhile work.

Nobody suggested that. It’s perfectly possible to “be polite” and still not delay worthwhile work.

  • The current policy is “downstream is on their own”.

Nobody disputes that.

  • There was nothing even remotely unreasonable done in the patch series which triggered this discussion and I don’t want any upstream contributor coming to believe there was.

I disagree with you about “nothing even remotely unreasonable” in that I think it was done slightly unreasonably, and I think it’s worthwhile to have other upstream contributors acknowledge that feeling.

What I’m aware of is a series of 5 separate patches, each of which re-capitalized a select subset of APIs in the same area. This fixing-up is long overdue and I’m very happy to see it done. But I will dispute the “reasonableness” of doing it in 5 separate stages (plus a few other related cleanups that were not simply re-capitalizing existing names).

The notion of “reviewable size” is not relevant, because these patches were not reviewed (in accordance with current policy, that this kind of mechanical fixup is “obvious” and does not require pre-commit review).

The patches were not really very distinct, with a HUGE overlap in the set of affected files.

We do like to minimize churn; this kind of fixup clearly qualifies as churn, but I respectfully submit that the way it was done does not minimize the churn.

And minimizing the churn is good for both the project and those of us who live downstream of it.

–paulr

Philip,

This statement “The current policy is “downstream is on their own”.” appears to have gotten out of hand.

LLVM for this thread is a production line, a pipeline, a team operation. Of course each person in the team has their own responsibility but there is never a team member who is on their own. There is never a position on the production line that does not depend on all the other positions in order to keep the production line running. At some point all the members depend on and are responsible for all the other members and for the success of the team.

This is not policy. It is merely a fact of life for this kind of organization.

Neil Nelson

Hi Philip,

I think you might be reading more into the suggestion/discussion than is actually there.

  • I do not want upstream developers “trying to be polite” if that delays otherwise worthwhile work.

Nobody suggested that. It’s perfectly possible to “be polite” and still not delay worthwhile work.

  • The current policy is “downstream is on their own”.

Nobody disputes that.

  • There was nothing even remotely unreasonable done in the patch series which triggered this discussion and I don’t want any upstream contributor coming to believe there was.

I disagree with you about “nothing even remotely unreasonable” in that I think it was done slightly unreasonably, and I think it’s worthwhile to have other upstream contributors acknowledge that feeling.

What I’m aware of is a series of 5 separate patches, each of which re-capitalized a select subset of APIs in the same area. This fixing-up is long overdue and I’m very happy to see it done. But I will dispute the “reasonableness” of doing it in 5 separate stages (plus a few other related cleanups that were not simply re-capitalizing existing names).

Yeah, I’m not sure how I’d have done it - imagine I’d have done it more likely at either extreme - renaming one function at a time (which probably would’ve been more disruptive than what happened, I guess) or the whole class.

The notion of “reviewable size” is not relevant, because these patches were not reviewed (in accordance with current policy, that this kind of mechanical fixup is “obvious” and does not require pre-commit review).

FWIW, post-commit review is still important to me (& revertability of patches, even those made through automated means - though, in the case of reformats/renames that can argue in favor of larger patches rather than having to figure out how to revert a whole series of dependent patches because the changes are very close together).

The patches were not really very distinct, with a HUGE overlap in the set of affected files.

I’m not sure the set of affected files weighs much in my mind in this - if the changes are sufficiently separated within those files.

We do like to minimize churn; this kind of fixup clearly qualifies as churn, but I respectfully submit that the way it was done does not minimize the churn.

Churn to me is changing and rechanging the same things - and sometimes “change for the sake of change”/large scale changes in general (with the concerns around difficult blame, etc) - the sort of changes that are likely to disrupt existing outstanding patches, etc. (perhaps that’s a good place where there’s overlap in motives/incentives - outstanding upstream patches suffer some of the same problems as downstream forks (essentially a very large amount of indefinitely outstanding patches) - and making choices that reduce the frequency/total amount of work to keep those patches up to date is generally a good thing, but is weighed against other benefits for sure)

With this kind of discussion, it's probably good to keep in mind that
every individual developer is really their own little downstream most
of the time.

Given how slow the LLVM review process is, many developers probably
have a few changes in flight most of the time. Gratuitous churn forces
them into rebases, as well. So "being nice to downstreams" and "being
nice to all LLVM developers" is often very well aligned and not in
conflict at all!

Cheers,
Nicolai

If you ignore downstream users, and focus on upstream: what makes it the least risky and most convenient for the upstream contributor in this case? It seems they believed it was to split their patch up, which I would have done as well: if you can split it, then in most case you should.

If the patches can land separately, to me their are distinct (they are not unrelated, but that’s not a reason for merging them in a single commit). Just like David mentioned separately, I may have done this over a much longer series of individual commits.

If you are arguing that the reason to not split these in separate patch is about downstream users, then I am with Philip: you start to creep up some expectations that are going against the current status quo in which (in my opinion) downstream shouldn’t impact upstream development.

Best,