On Mon, Jul 15, 2013 at 11:50 AM, Luke Wagner <[email protected]> wrote:
> Thanks for taking the time to write this up! (I was on PTO for the week so
> I'm just seeing this now.) It seems like there haven't been too many
> comments which I'll assume means mostly agreement from the group?
This mail was affected by the mail outage so I never got them.
>
>> 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...
>
> Usually we make exceptions for big patches (since it is hard enough to land
> these already in the face of a fast-moving tree), but it seems like the
> proposed rules here still provide leeway but make it a bit more structured
> than before. In particular, landing a big patch into a dogpile or right
> before an uplift is usually a bad idea anyway.
>
>> 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.
>
> It would be good to be explicit about this: elevating these benchmarks from
> 'Breakdown >> Assorted tests' to 'Breakdown >> Other important' on awfy
> (creating a new dir in awfy: benchmarks/misc/tests/important).
>
> When this discussion settles (if it hasn't already), could you post the
> guidelines up on
> https://wiki.mozilla.org/index.php?title=JavaScript:SpiderMonkey:Regression_Policy
> ?
>
> Cheers,
> Luke
> _______________________________________________
> dev-tech-js-engine-internals mailing list
> [email protected]
> https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
_______________________________________________
dev-tech-js-engine-internals mailing list
[email protected]
https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
