RFC: Owning Bitcode

Hello,

Chris’s “keynote” at the LLVM Developers’ Conference included a call for code owners, and my company has a heavy dependency on Bitcode, I propose taking ownership of:

lib/Bitcode/*
include/Bitcode/*

This means that I’ll be committed to documenting (yay) the implementation and responsible for reviewing patches and commits, as well as overall code quality and maintenance. It’s a small section of the overall tree, and I realize that it is a critical portion that touches virtually the whole project.

If someone else already “owns” this part, please let me know. Looking at the commits of BitcodeReader.cpp, the folks that have the most invested are:

lattner - 101
djg - 32
dpatel - 30
void - 27
resistor - 22
johannes - 14
nicholas - 12
foad - 12
hernande - 10
baldrick - 9

I <3 bitcode and would love to be responsible for it. That being said, my next step would be to prove it through writing documentation and submitting more patches that align with the design philosophies of the project.

Thanks for your attention!

Joe

Hello,

Chris’s “keynote” at the LLVM Developers’ Conference included a call for code owners, and my company has a heavy dependency on Bitcode, I propose taking ownership of:

lib/Bitcode/*
include/Bitcode/*

This means that I’ll be committed to documenting (yay) the implementation and responsible for reviewing patches and commits, as well as overall code quality and maintenance. It’s a small section of the overall tree, and I realize that it is a critical portion that touches virtually the whole project.

This sounds like a great idea to me. Unless someone objects in the next few days, consider it yours :). Please update docs/DeveloperPolicy to list yourself in the owners section. Thanks Joe,

-Chris

Hi Joe,

Chris's "keynote" at the LLVM Developers' Conference included a call for code
owners, and my company has a heavy dependency on Bitcode, I propose taking
ownership of:

lib/Bitcode/*
include/Bitcode/*

This means that I'll be committed to documenting (yay) the implementation and
responsible for reviewing patches and commits, as well as overall code quality
and maintenance.

just to be clear, I don't think you are obliged to do everything yourself, you
can always ask someone else to review a particular patch and so on. It's more
that your job is to ensure that patches do get timely review, that documentation
does get written, the code is cleaned and refactored etc.

Ciao, Duncan.

Is there a particular sub-system size that makes sense to mark as owned? I have been reworking the library call
simplification infrastructure recently and will be happy to sign up as an owner for that.

I think that "directory level" is the right granularity. If you're interested in signing up to maintain and review the whole instcombine library, that would be a great level. To see what this entails, see:
http://llvm.org/docs/DeveloperPolicy.html#code-owners

-Chris

I am interested and comfortable with that. I will wait a few days and then
update CODE_OWNERS.TXT.

Hi,

Is there a particular sub-system size that makes sense to mark as owned? I have been
reworking the library call simplification infrastructure recently and will be happy
to sign up as an owner for that.

I think that "directory level" is the right granularity. If you're interested in signing
up to maintain and review the whole instcombine library, that would be a great level. To
see what this entails, see:
http://llvm.org/docs/DeveloperPolicy.html#code-owners

I am interested and comfortable with that. I will wait a few days and then
update CODE_OWNERS.TXT.

while I think it's great that Meador is stepping forward to help out here, it
does bring up the question of how expert you have to be to become a code owner.
As far as I know (please correct me if I'm wrong) Meador isn't the number 1 LLVM
instcombine expert, but instead is someone who knows instcombine fairly well
and is willing to spend time making sure instcombine is in good shape and stays
that way. My take is that you should be able to be a code owner without being
the ultimate über hacker for that subsystem, since (1) the über hackers will
have their eye on you and will let you know if you get it wrong; and (2) being
willing counts for a lot. More power to volunteers! Hopefully if people talk
with each other, help each other out, and maintain a friendly and cooperative
atmosphere then all problems with code ownership will just come out in the wash.

However another take on the whole code owner thing is that the code owner is
the person who has the final say on what goes in and what doesn't, so it is
important for the code owner to be the über guru (and having other people
override their decisions would just undermine their authority and the whole
code owner system). Personally I don't really buy this, but I can understand
the concern.

Maybe Chris should give his take on the whole code owner thing?

Ciao, Duncan.

+1

I think enthusiasm (paired with good knowledge) counts a lot.

That is definitely a fair assessment. I am not the #1 instcombine
super-hacker, but I am willing to keep it in good shape. Even if that means
bugging other people :slight_smile:

Another point to address is whether there can be more than one owner in a
particular area.

There might be more than one code expert per area, which can be
referenced by the code owner, but I wouldn't have more than one owner.

Owning part of the code means you'll have to keep a sharper eye on
particular changes than others and should raise concerns when they're
due, and not to take the blame for breaking or charged for fixing
broken code.

On the topic of code owners:

Would it make sense to add target-specific code owners for the codegen targets? Some of the targets are more esoteric (e.g. NVPTX, CellSPU, AMDGPU) and may benefit from more fine-grained code ownership.

More power to volunteers! Hopefully if people talk

with each other, help each other out, and maintain a friendly and cooperative
atmosphere then all problems with code ownership will just come out in the wash.

+1 I think this is where we want the community to be, and the role of the code owners should
hopefully be minimal.

However another take on the whole code owner thing is that the code owner is
the person who has the final say on what goes in and what doesn't, so it is
important for the code owner to be the über guru (and having other people
override their decisions would just undermine their authority and the whole
code owner system). Personally I don't really buy this, but I can understand
the concern.

I don't think this is the role of the code owner. The way I understand the developer policy
is that the code owner is there to ensure that all patches are reviewed. I would go further
and say that the code owner is a fail-safe, which means if the code review falls back on the code
owner, the regular review process has failed, and the code owner will have to take some measures
(any suggestions?) to make people review patches in a timely manner. It will be too easy for people
to stop reviewing, since they now think there is a code owner, which will do all code reviews, and the
result might be that person will sooner or later be overwhelmed and drop out.

It would perhaps be better to have the code owner be someone that has the knowledge, and cares
about a specific component in the context of quality, capability, design and documentation. But it should
probably not be the person who is the most frequent contributor to that component (might be difficult to find
such a person), since since the reviews may be likely to fall back on the same person, and he/she will have
to keep bugging other people about their own patches, or more likely, patches will go in without review if
the code owner has a review-after-commit status.

- Jan

It definitely makes sense to have code owners for each subdirectory of lib/Target.

-Chris

More power to volunteers! Hopefully if people talk

with each other, help each other out, and maintain a friendly and cooperative
atmosphere then all problems with code ownership will just come out in the wash.

+1 I think this is where we want the community to be, and the role of the code owners should
hopefully be minimal.

However another take on the whole code owner thing is that the code owner is
the person who has the final say on what goes in and what doesn't, so it is
important for the code owner to be the über guru (and having other people
override their decisions would just undermine their authority and the whole
code owner system). Personally I don't really buy this, but I can understand
the concern.

I don't think this is the role of the code owner. The way I understand the developer policy
is that the code owner is there to ensure that all patches are reviewed. I would go further
and say that the code owner is a fail-safe, which means if the code review falls back on the code
owner, the regular review process has failed,

Not exactly - since this includes post commit review too. Since we
don't have any positive acknowledgement of post-commit review (when
there are no comments being provided) a code owner will generally end
up having to perform any cases of post-commit review in there area
(since they have no way of knowing that someone else also did so if
they provided no feedback).

Certainly if a code owner comes up with some way to manage the
post-commit review of their trusted reviewers using a shared queue,
that could be a way to alleviate some of this load.

(& perhaps it goes without saying, but the owner's responsibility
isn't just to ensure code gets reviewed, but that it gets reviewed by
someone appropriately skilled/knowledgeable in the area)

First: this is a relatively minor concern in the grander scheme of
things. I generally don't think it will come up often and will get
sorted out correctly if it does. However, when it does come up,
without being addressed, i suspect it will take a fair amount of time
and energy of the community to sort it all out. So maybe it's worth
addressing it now....

Personally, the reason for this concern is the conflation of several
independent roles into a single "code ownership" bucket:
- What the development policy says: ensuring each patch gets some review.
- What the release process follows: code owners get the final say in
whether a patch gets cherry picked into a release.
- What has happened in practice: code owners make decisions when there
are two viable choices with differing pros and cons, but no clear
consensus on which direction is preferred.

I think your first paragraph's process works very well for the first,
"ok" for the second, and not very well for the third. I think for the
third point (and to a lesser degree for the second), what you want is
*an* expert in the area, not necessarily *the* expert. Interestingly,
if we can address the third point, that also solves the only tricky
part of the second point: if there is a concern or lack of consensus
when bringing a patch into the release, the second point can magically
be promoted to the third.

I see two easy ways to avoid this issue:

1) Split off the third point from code ownership. Have some other
mechanism, which hopefully is not a single-point-of-failure (or
success... ;]).

2) Require code owners to be at least one of the "trusted maintainers"
of the area they own (to quote the dev policy).

I think either one of these would work quite well, and I have no real
preference for which we follow. #1 would be most consistent with the
wording of the documentation, and #2 would be most consistent with
existing precedent and activities of the community.

Is there a particular sub-system size that makes sense to mark as owned? I have been
reworking the library call simplification infrastructure recently and will be happy
to sign up as an owner for that.

I think that "directory level" is the right granularity. If you're interested in signing
up to maintain and review the whole instcombine library, that would be a great level. To
see what this entails, see:
http://llvm.org/docs/DeveloperPolicy.html#code-owners

I am interested and comfortable with that. I will wait a few days and then
update CODE_OWNERS.TXT.

while I think it's great that Meador is stepping forward to help out here, it
does bring up the question of how expert you have to be to become a code owner.
As far as I know (please correct me if I'm wrong) Meador isn't the number 1 LLVM
instcombine expert, but instead is someone who knows instcombine fairly well
and is willing to spend time making sure instcombine is in good shape and stays
that way.

Right.

My take is that you should be able to be a code owner without being
the ultimate über hacker for that subsystem, since (1) the über hackers will
have their eye on you and will let you know if you get it wrong; and (2) being
willing counts for a lot. More power to volunteers! Hopefully if people talk
with each other, help each other out, and maintain a friendly and cooperative
atmosphere then all problems with code ownership will just come out in the wash.

I agree. I also think that "knowing your own limitations" is an important part of the job. If Meador (or any other code owner) doesn't feel comfortable with something, feels like a patch is questionable, or is outside the domain of his expertise, then he should start a discussion about the topic. He isn't the one that has to "know everything" so much as make sure that InstCombine is getting love, the patches are getting attention, and that people aren't doing things that are against the architecture of Instcombine.

Also, having a specific code owner doesn't mean that everyone else should stop paying attention to the patches :). Finally, if someone isn't working out well as a code owner, I'm confident that it will be recognized and they will gracefully bow out of the position.

However another take on the whole code owner thing is that the code owner is
the person who has the final say on what goes in and what doesn't, so it is
important for the code owner to be the über guru (and having other people
override their decisions would just undermine their authority and the whole
code owner system). Personally I don't really buy this, but I can understand
the concern.

I don't really see it this way. If a code owner wants "thing A" to happen and much of the community is strongly in favor of "thing B" happening, then it should be discussed until there is some agreement about what should happen. If no resolution can be had, I am happy to personally help resolve the dispute.

-Chris

Personally, the reason for this concern is the conflation of several
independent roles into a single "code ownership" bucket:
- What the development policy says: ensuring each patch gets some review.
- What the release process follows: code owners get the final say in
whether a patch gets cherry picked into a release.
- What has happened in practice: code owners make decisions when there
are two viable choices with differing pros and cons, but no clear
consensus on which direction is preferred.

I think this is a good breakdown, but it is also listed in descending order of frequency. The third occurrence is actually pretty rare, and no matter who the code owner is, these events are full of long discussions with many experts.

I see two easy ways to avoid this issue:

1) Split off the third point from code ownership. Have some other
mechanism, which hopefully is not a single-point-of-failure (or
success... ;]).

In my opinion, this is already true in practice, and the task is delegated to the general community at large. This is the point of the discussion.

2) Require code owners to be at least one of the "trusted maintainers"
of the area they own (to quote the dev policy).

Given that many of the "trusted maintainers" are not willing or able to be a code owner, I don't think this is a great solution. It means that *I* end up de-facto owning everything, which isn't good for the community either.

-Chris

Not exactly - since this includes post commit review too. Since we

don't have any positive acknowledgement of post-commit review (when
there are no comments being provided) a code owner will generally end
up having to perform any cases of post-commit review in there area
(since they have no way of knowing that someone else also did so if
they provided no feedback).

Certainly if a code owner comes up with some way to manage the
post-commit review of their trusted reviewers using a shared queue,
that could be a way to alleviate some of this load.

I think it would be good policy to have anyone doing a formal (thorough) review
send a mail to the code owner with comments (or an OK). Code owners can
then organize reviewers the way they want.
Keeping track of what has been reviewed is essential, since the
majority of commits (I am guessing) are post-commit reviews. Having a person
quickly scan over a patch, especially if it ends up being a self-review in case
the code owner is committing code, is unlikely to improve things. I don't want
a heavy-weight process, but if people are going to come in and drop out etc,
there has to be some way of keeping track of things.

- Jan

Phabricator provides a way to set up notifications for changes to
certain parts of the codebase & you can then "accept" those changes
which is probably the right way to do this, but I haven't tried the
workflow myself.

- David

I currently use this and find it works quite well. The only issues
with it that I've found are that everyone has to be registered and
have the same username as svn for "raise concern" to alert them. The
other is that you still get an audit even if the patch was approved
pre-commit.

The first issue is already being worked on. The second isn't really a
generally solvable problem unless you include the phabricator
Differential number in the commit message, as quite a few reviews end
with "change x and commit".

- Michael Spencer