Re: A new testing policy for code landing in mozilla-central
> On Sep 24, 2020, at 7:02 AM, Kartikaya Gupta wrote: > > First - thank you for making these changes! I do find the testing > prompt useful as a reviewer, and it has already resulted in a few > extra tests being added where they probably wouldn't have been > otherwise. > > After living with this for a week or so, I have two (relatively minor) > papercuts: Thanks for the feedback: > - I really want the webextension available in Fenix. I do a surprising > number of reviews from my phone and having to manually add the tags is > annoying A shorter term goal would be to ship a Phabricator extension that implements the web extension behavior so you don’t need to install anything. Longer term it'd be great if the UI more integrated into the Accept process rather than being driven through the Project Tags UI (for example, a radio list to select from before you can click Submit). Of course getting a Phabricator extension rolled out for everyone has a much higher bar for testing and quality compared with the web extension, and needs to be balanced with everything else on that team's roadmap. But it's good motivation to hear you are finding the web extension useful. > - Sometimes I want to mark a patch "testing-exception-elsewhere" or > "testing-exception-other" but also leave a general review comment. It > seems awkward to me to have to mix my general review comments with the > testing-specific comment into the same input field, and I'm always > worried it will confuse the patch author. If this process is > eventually formalized into something more structured than phabricator > project tags it would be nice to have separate fields for the > testing-related comment and the general review comment. Yeah, keeping a structured field with the testing policy comment would also be great for querying or scanning revisions. Maybe something in the revision details - though I’d want the ability to comment more integrated than having to go to Edit Revision as a separate step _after_ adding the Project Tag. I've pinged Zeid to ask if there's any existing functionality along these lines. > Cheers, > kats > > On Tue, Sep 15, 2020 at 11:03 AM Brian Grinstead > wrote: >> >> (This is a crosspost from firefox-dev) >> >> Hi all, >> >> We’re rolling out a change to the review process to put more focus on >> automated testing. This will affect you if you review code that lands in >> mozilla-central. >> >> ## TLDR >> >> Reviewers will now need to add a testing Project Tag in Phabricator when >> Accepting a revision. This can be done with the “Add Action” → “Change >> Project Tags” UI in Phabricator. There's a screenshot of this UI at >> https://groups.google.com/g/firefox-dev/c/IlHNKOKknwU/m/zl6cOlT2AgAJ. >> >> We’ve been piloting the policy for a few weeks with positive feedback from >> reviewers. Still, there may be some rough edges as we roll this out more >> widely. I’d like to hear about those, so please contact me directly or in >> the #testing-policy channel in Slack / Matrix if you have feedback or >> questions about how the policy should apply to a particular review. >> >> We’re working on ways to make this more convenient in the UI and to prevent >> landing code without a tag in Lando. In the meantime if you'd like a >> reminder to add the tag while reviewing code, Nicolas Chevobbe has built a >> WebExtension that automatically opens up the Project Tags UI when >> appropriate at https://github.com/nchevobbe/phab-test-policy. >> >> ## Why? >> >> Automated tests in Firefox and Gecko reduce risk and allow us to quickly and >> confidently improve our products. >> >> While there’s a general understanding that changes should have tests, this >> hasn’t been tracked or enforced consistently. We think defining an explicit >> policy for including automated tests with code changes will help to achieve >> those goals. >> >> Also, we’ll be able to better understand exactly why and when tests aren’t >> being added. This can help to highlight components that need new testing >> capabilities or technical refactoring, and features that require increased >> manual testing. >> >> There are of course trade-offs to enforcing a testing policy. Tests take >> time to write, maintain, and run which can slow down day-to-day development. >> And given the complexity of a modern web browser, some components are >> impractical to test today (for example, components that interact with >> external hardware and software). >> >> The policy below hopes to mitigate those by using a lightweight enforcement >> mechanism and the ability to exempt changes at the discretion of the code >> reviewer. >> >> ## Policy (This text is also located at >> https://firefox-source-docs.mozilla.org/testing/testing-policy/) >> >> Everything that lands in mozilla-central includes automated tests by >> default. Every commit has tests that cover every major piece of >> functionality and expected input conditions. >>
Re: Dogfooding Warp
That's an impressive speedup! Congrats on enabling this, everyone. On 24/09/2020 14:56, Jan de Mooij wrote: Warp is now enabled by default on Nightly, after positive feedback from users dogfooding it [0,1]. Here are just a few of the Talos/Raptor graphs showing improvements when Warp landed: - 20% on Win64 GDocs loadtime: https://mzl.la/3cp6dAs - 13% on Android Reddit SpeedIndex: https://mzl.la/2RUWdp8 - 18% on pdfpaint: https://mzl.la/2HtXb9W - 8% on tp6 JS memory: https://mzl.la/3j2VwGb - 8% on damp (devtools perf): https://mzl.la/3kLbhSM Please let us know if you notice any improvements or regressions. Thanks, The Warp team [0] https://www.reddit.com/r/firefox/comments/itib6s/dogfooding_warp_on_nightly_new_js_jit_engine/ [1] https://www.reddit.com/r/firefox/comments/iy2036/nightly_is_finally_feeling_as_fast_as_chromium/ On Tue, Sep 15, 2020 at 2:57 PM Jan de Mooij wrote: Hi all, The SpiderMonkey (JS) team has been working on a significant update to our JITs called WarpBuilder (or just Warp) [0,1]. Before we enable Warp by default in Nightly (hopefully next cycle in 83) we need your help dogfooding it. Warp improves performance by reducing the amount of internal type information that is tracked, optimizing for a broader spectrum of cases, and by leveraging the same CacheIR optimizations used by last year’s BaselineInterpreter work [2]. As a result, Warp has a much simpler design and improves responsiveness and page load performance significantly (we're seeing 5-15% improvements on many visual metrics tests). Speedometer is about 10% faster with Warp. The JS engine also uses less memory when Warp is enabled. To enable Warp in Nightly: 1. Update to a recent Nightly 2. Go to about:config and set the "javascript.options.warp" pref to true 3. Restart the browser We're especially interested in stability issues and real-world performance problems. Warp is currently slower on various synthetic JS benchmarks such as Octane (which we will continue investigating in the coming months) but should perform well on web content. If you find any issues, please file bugs blocking: https://bugzilla.mozilla.org/show_bug.cgi?id=1613592 If you notice any improvements, we'd love to hear about those too. Finally, we want to thank our amazing contributors André Bargull and Tom Schuster for their help implementing and porting many optimizations. Turning Warp on is only our first step, and we expect to see a lot of new optimization work over the next year as we build on this. We are excited for what the future holds here. Thanks! The Warp team [0] WarpBuilder still utilizes the backend of IonMonkey so we don't feel it has earned the WarpMonkey name just yet. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1613592 [2] https://hacks.mozilla.org/2019/08/the-baseline-interpreter-a-faster-js-interpreter-in-firefox-70/ ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: A new testing policy for code landing in mozilla-central
First - thank you for making these changes! I do find the testing prompt useful as a reviewer, and it has already resulted in a few extra tests being added where they probably wouldn't have been otherwise. After living with this for a week or so, I have two (relatively minor) papercuts: - I really want the webextension available in Fenix. I do a surprising number of reviews from my phone and having to manually add the tags is annoying - Sometimes I want to mark a patch "testing-exception-elsewhere" or "testing-exception-other" but also leave a general review comment. It seems awkward to me to have to mix my general review comments with the testing-specific comment into the same input field, and I'm always worried it will confuse the patch author. If this process is eventually formalized into something more structured than phabricator project tags it would be nice to have separate fields for the testing-related comment and the general review comment. Cheers, kats On Tue, Sep 15, 2020 at 11:03 AM Brian Grinstead wrote: > > (This is a crosspost from firefox-dev) > > Hi all, > > We’re rolling out a change to the review process to put more focus on > automated testing. This will affect you if you review code that lands in > mozilla-central. > > ## TLDR > > Reviewers will now need to add a testing Project Tag in Phabricator when > Accepting a revision. This can be done with the “Add Action” → “Change > Project Tags” UI in Phabricator. There's a screenshot of this UI at > https://groups.google.com/g/firefox-dev/c/IlHNKOKknwU/m/zl6cOlT2AgAJ. > > We’ve been piloting the policy for a few weeks with positive feedback from > reviewers. Still, there may be some rough edges as we roll this out more > widely. I’d like to hear about those, so please contact me directly or in the > #testing-policy channel in Slack / Matrix if you have feedback or questions > about how the policy should apply to a particular review. > > We’re working on ways to make this more convenient in the UI and to prevent > landing code without a tag in Lando. In the meantime if you'd like a reminder > to add the tag while reviewing code, Nicolas Chevobbe has built a > WebExtension that automatically opens up the Project Tags UI when appropriate > at https://github.com/nchevobbe/phab-test-policy. > > ## Why? > > Automated tests in Firefox and Gecko reduce risk and allow us to quickly and > confidently improve our products. > > While there’s a general understanding that changes should have tests, this > hasn’t been tracked or enforced consistently. We think defining an explicit > policy for including automated tests with code changes will help to achieve > those goals. > > Also, we’ll be able to better understand exactly why and when tests aren’t > being added. This can help to highlight components that need new testing > capabilities or technical refactoring, and features that require increased > manual testing. > > There are of course trade-offs to enforcing a testing policy. Tests take time > to write, maintain, and run which can slow down day-to-day development. And > given the complexity of a modern web browser, some components are impractical > to test today (for example, components that interact with external hardware > and software). > > The policy below hopes to mitigate those by using a lightweight enforcement > mechanism and the ability to exempt changes at the discretion of the code > reviewer. > > ## Policy (This text is also located at > https://firefox-source-docs.mozilla.org/testing/testing-policy/) > > Everything that lands in mozilla-central includes automated tests by default. > Every commit has tests that cover every major piece of functionality and > expected input conditions. > > One of the following Project Tags must be applied in Phabricator before > landing, at the discretion of the reviewer: > > * `testing-approved` if it has sufficient automated test coverage. > * One of `testing-exception-*` if not. After speaking with many teams across > the project we’ve identified the most common exceptions, which are detailed > below. > > ### Exceptions > > * testing-exception-unchanged: Commits that don’t change behavior for end > users. For example: > * Refactors, mechanical changes, and deleting dead code as long as they > aren’t meaningfully changing or removing any existing tests. Authors should > consider checking for and adding missing test coverage in a separate commit > before a refactor. > * Code that doesn’t ship to users (for example: documentation, build > scripts and manifest files, mach commands). Effort should be made to test > these when regressions are likely to cause bustage or confusion for > developers, but it’s left to the discretion of the reviewer. > * testing-exception-ui: Commits that change UI styling, images, or localized > strings. While we have end-to-end automated tests that ensure the frontend > isn’t totally broken, and screenshot-based tracking of changes
Re: Dogfooding Warp
Warp is now enabled by default on Nightly, after positive feedback from users dogfooding it [0,1]. Here are just a few of the Talos/Raptor graphs showing improvements when Warp landed: - 20% on Win64 GDocs loadtime: https://mzl.la/3cp6dAs - 13% on Android Reddit SpeedIndex: https://mzl.la/2RUWdp8 - 18% on pdfpaint: https://mzl.la/2HtXb9W - 8% on tp6 JS memory: https://mzl.la/3j2VwGb - 8% on damp (devtools perf): https://mzl.la/3kLbhSM Please let us know if you notice any improvements or regressions. Thanks, The Warp team [0] https://www.reddit.com/r/firefox/comments/itib6s/dogfooding_warp_on_nightly_new_js_jit_engine/ [1] https://www.reddit.com/r/firefox/comments/iy2036/nightly_is_finally_feeling_as_fast_as_chromium/ On Tue, Sep 15, 2020 at 2:57 PM Jan de Mooij wrote: > > Hi all, > > The SpiderMonkey (JS) team has been working on a significant update to > our JITs called WarpBuilder (or just Warp) [0,1]. Before we enable > Warp by default in Nightly (hopefully next cycle in 83) we need your > help dogfooding it. > > Warp improves performance by reducing the amount of internal type > information that is tracked, optimizing for a broader spectrum of > cases, and by leveraging the same CacheIR optimizations used by last > year’s BaselineInterpreter work [2]. As a result, Warp has a much > simpler design and improves responsiveness and page load performance > significantly (we're seeing 5-15% improvements on many visual metrics > tests). Speedometer is about 10% faster with Warp. The JS engine also > uses less memory when Warp is enabled. > > To enable Warp in Nightly: > > 1. Update to a recent Nightly > 2. Go to about:config and set the "javascript.options.warp" pref to true > 3. Restart the browser > > We're especially interested in stability issues and real-world > performance problems. Warp is currently slower on various synthetic JS > benchmarks such as Octane (which we will continue investigating in the > coming months) but should perform well on web content. > > If you find any issues, please file bugs blocking: > > https://bugzilla.mozilla.org/show_bug.cgi?id=1613592 > > If you notice any improvements, we'd love to hear about those too. > > Finally, we want to thank our amazing contributors André Bargull and > Tom Schuster for their help implementing and porting many > optimizations. > > Turning Warp on is only our first step, and we expect to see a lot of > new optimization work over the next year as we build on this. We are > excited for what the future holds here. > > Thanks! > The Warp team > > [0] WarpBuilder still utilizes the backend of IonMonkey so we don't > feel it has earned the WarpMonkey name just yet. > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1613592 > [2] > https://hacks.mozilla.org/2019/08/the-baseline-interpreter-a-faster-js-interpreter-in-firefox-70/ ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
