Brian and I had the unfortunate experience this Monday that the rules regarding the required or allowed actions with regard to revisions that cause performance regressions aren’t well enough defined. Or at least that there is some room for improvement. So the intention of this post is for everyone to agree and/or improve the rules regarding regressions.
So first of all regressions are bad and we should try to avoid them. Getting them as fast as possible out of the tree is the easiest and best way to make sure they don’t stick or become impossible to backout. Another issue with such revisions in the tree is that they possibly obscure other regressions. The default action with this is to backout. Dave Mandelin was very clear about this and allowed everybody to just backout if a revision regresses our performance on one of our big benchmarks (SS, Kraken, Octane). Now the common practice I observed during my time here is as follows: 1) If one of the big benchmarks is hurt, the relevant person is notified and asked for a really short term solution. The action in most cases was rushing a fix or backing the revision out of the tree. 2) If one of the other benchmarks are hurt, in most cases nothing happens or a regression bug is opened depending on the bug causing the regression. So there is a big difference between the common practice and the actual requested actions. And in most cases the common practice works fine. But not always. This weekend we had 4 regressions at the same time. It is rather easy to pinpoint one revision, but 2 is already a whole other story, let alone 4 regressions! When that happens the rule about backing out regressions makes it much easier to get the tree in a good state and to make sure all regressions are found... Afterwards the required fixes can get made or the discussion can begin to allow a onetime regression ... So since both policies/guidelines aren’t quite up to the task of facilitating tree watchers to take actions and to annoy the person whose patch is backed out as little as possible, I have sort of a new proposal, based on common occurrences regarding regressions. My proposal would be something like: ----------------------------------------------------------------------- Revisions causing regressions on the major benchmarks (Sunspider, Octane, Kraken) are not allowed at all. Except: if the regressions are listed in the bug report and the reviewer still gave a r+, knowing about these regressions. (e.g. common case where this happens is if a benchmark as a whole improves, but some subtests show regressions) Normal procedure (i.e. only one regression): 1) The owner is notified and the regressions are listed in the bug 2) The owner should immediately acknowledge the regressions and inform what his plan of attack is. 3) The owner gets 2 working days to fix them: 3.1.) Backout 3.2.) Fix in tree 3.3.) Request r+ to allow regressions (only allowed in extreme cases!) Note: if the process takes or will take longer than 2 working days, the patch should be backed out. Allowed actions in exceptional cases (multiple regressions, just before merge …) 1) The revision is backed out without notifying the person beforehand 2) The backout and the list of regressions is listed in the bug + a reason why a quick backout was done instead of the normal procedure Note: this shouldn’t be frowned upon. If the regression was listed in the bug before r+ it wouldn’t have gotten backed out... ------------------------------------------------------------------------- As an extra discussion point, I would like to have some benchmarks being classified as more important. To disallow regression on them. Not like our major benchmarks where “no regressions allowed”, but benchmarks where we discourage regressions... A few benchmarks have proven to be also important even though they are not included in major benchmark set. One of them is e.g. jpeg2000 benchmark. Possible our performance is as important as the pdf.js benchmark. Let the discussions begin ... _______________________________________________ dev-tech-js-engine-internals mailing list [email protected] https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
