On Tue, Aug 17, 2021 at 1:46 PM Ken Rockot <[email protected]> wrote:
> On Tue, Aug 17, 2021 at 1:31 PM Dirk Pranke <[email protected]> wrote: > >> I'm not entirely sure why you think this would be preferable to do inside >> GN. >> >> I can see the thinking that says "this is where we list everything else >> that is needed at runtime" (and that makes sense to me), but are there >> other reasons? >> > > I would expect it to ~eliminate the running cost of collecting tests from > the filesystem, while also avoiding the introduction of yet another > generated file to be checked in (probably with lots of contention). > Sure. > It's work that can be done at build time, but only if we have sufficient > build metadata. > Yeah, if we're willing to make "every test must be listed in GN" a requirement that's certainly doable. We don't necessarily have to list every *file* (i.e., every baseline, import, etc.) though there may be advantages to doing so (there are clearly also disadvantages to doing so). It's unclear to me whether doing that would be worth the cost in increased GN runtime, but we could look into it. -- Dirk > >> -- Dirk >> >> On Tue, Aug 17, 2021 at 12:36 PM Ken Rockot <[email protected]> wrote: >> >>> This does sound like a great problem to solve btw, and it's wonderful to >>> see work being done on it. >>> >>> Since we now have this nice script to collect all the tests, could we >>> try to split it into two pieces? My thinking would be: >>> >>> - One piece becomes a run-once-ever script to collect all the tests >>> from the filesystem and emit a tree of BUILD.gn files with targets to >>> list >>> test sources (and subdirs as deps) explicitly >>> - The other piece becomes a build action which aggregates test >>> sources from aforementioned build targets and spits out this unified JSON >>> file as a proper build artifact >>> >>> I'm sure this can be done with GN, the only question would be as Dirk >>> points out, whether or not it might explode buildgen time. Even if it does, >>> maybe that's something we try to solve in GN? >>> >>> On Tue, Aug 17, 2021 at 12:00 PM 'Dirk Pranke' via blink-dev < >>> [email protected]> wrote: >>> >>>> I wouldn't particularly want to make this a standard step on the bots, >>>> because that's just not really how the code is structured. >>>> >>>> We could potentially do something as part of the isolate step. The >>>> code's not really structured for that, either, but that would be less >>>> distasteful than adding a separate step. >>>> >>>> -- Dirk >>>> >>>> On Tue, Aug 17, 2021 at 11:47 AM Xianzhu Wang <[email protected]> >>>> wrote: >>>> >>>>> Can we make this a standalone step or run it in the "isolate test" >>>>> step on the bot? It's like a preparation before sharding blink_web_tests, >>>>> to reduce the duplicated work on the swarming bots. >>>>> >>>>> On Tue, Aug 17, 2021 at 11:17 AM Domenic Denicola < >>>>> [email protected]> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Tue, Aug 17, 2021 at 2:08 PM 'Weizhong Xia' via blink-dev < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> Running collect_web_tests on external/wpt is kind of slow, because >>>>>>> we need to generate wpt manifest first. Running that on other folders >>>>>>> will >>>>>>> be fast. And I will take a look to see if there is room for improvement. >>>>>>> >>>>>>> One thing worth mentioning is that if the change in external/wpt is >>>>>>> meant to be exported to upstream >>>>>>> <https://github.com/web-platform-tests/wpt>, you can ignore the >>>>>>> presubmit check warning. (Should have mentioned this in the PSA). >>>>>>> >>>>>> >>>>>> Aren't all changes in external/wpt meant to be exported to upstream? >>>>>> From what I understand that's the purpose of the directory... >>>>>> >>>>>> >>>>>>> >>>>>>> On Tue, Aug 17, 2021 at 11:01 AM Dirk Pranke <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> On Tue, Aug 17, 2021 at 10:48 AM Ian Kilpatrick < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> So I was a little surprised by this last night. >>>>>>>>> >>>>>>>> >>>>>>>> Yeah, we probably should've had the discussion first, sorry :(. >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> 1. This was relatively slow on my macbook, running locally takes >>>>>>>>> ~40s. From just now: >>>>>>>>> >>>>>>>>> ikilpatrick-macbookpro:src ikilpatrick$ touch >>>>>>>>> third_party/blink/web_tests/external/wpt/css/css-tables/test.html >>>>>>>>> >>>>>>>>> ikilpatrick-macbookpro:src ikilpatrick$ time >>>>>>>>> third_party/blink/tools/collect_web_tests.py external/wpt >>>>>>>>> >>>>>>>>> >>>>>>>>> real 0m40.492s >>>>>>>>> >>>>>>>>> user 0m23.007s >>>>>>>>> >>>>>>>>> sys 0m41.715s >>>>>>>>> >>>>>>>>> 2. I had a bunch of uncommitted tests in various wpt directories >>>>>>>>> that I needed to move out to somewhere else (rather than the script >>>>>>>>> just >>>>>>>>> adding committed tests). >>>>>>>>> >>>>>>>> >>>>>>>> One other concern is that we might end up with a lot of merge >>>>>>>> conflicts in a single file, and so we might want to shard by the top >>>>>>>> level >>>>>>>> directories or something ... >>>>>>>> >>>>>>>> (I'd also note that uploading patches is already relatively slow, >>>>>>>>> do we have metrics for how long developers are waiting for upload >>>>>>>>> scripts >>>>>>>>> to run? - I suspect this is due to WPT presubmit scripts but unsure) >>>>>>>>> >>>>>>>> >>>>>>>> The upload checks are just generally crazy slow these days. I'm not >>>>>>>> sure if we have collected metrics for them, but things are an order of >>>>>>>> magnitude slower than I'd like them to be. >>>>>>>> >>>>>>>> But that's partially a separate topic (apart from this check, of >>>>>>>> course). >>>>>>>> >>>>>>>> -- Dirk >>>>>>>> >>>>>>>> >>>>>>>>> Ian >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Tue, Aug 17, 2021 at 10:20 AM 'Dirk Pranke' via blink-dev < >>>>>>>>> [email protected]> wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Tue, Aug 17, 2021 at 10:08 AM 'Weizhong Xia' via blink-dev < >>>>>>>>>> [email protected]> wrote: >>>>>>>>>> >>>>>>>>>>> tl;dr: We introduced a mechanism that pre-collects test cases to >>>>>>>>>>> //third_party/blink/web_tests/AllTestsByDirectories.json, and uses a >>>>>>>>>>> presubmit check to ensure this file is up to date. You can stop >>>>>>>>>>> here if you >>>>>>>>>>> don't write web test cases. >>>>>>>>>>> >>>>>>>>>>> Dear blink-devs, >>>>>>>>>>> >>>>>>>>>>> You may have noticed that when you try to upload a CL, you are >>>>>>>>>>> prompted to update AllTestsByDirectories.json if your CL changes >>>>>>>>>>> web test >>>>>>>>>>> cases. Yes, today we landed a CL >>>>>>>>>>> <https://chromium-review.googlesource.com/c/chromium/src/+/3067372> >>>>>>>>>>> that pre-collects test cases and saves that to the json file. The >>>>>>>>>>> reason to >>>>>>>>>>> do this is that rwt tries to collect tests on every swarming bots, >>>>>>>>>>> and that >>>>>>>>>>> takes about 30 seconds (the best case when running on SSD). This is >>>>>>>>>>> not a >>>>>>>>>>> small amount of time as we are targeting a 10-min CQ. After this CL >>>>>>>>>>> lands, >>>>>>>>>>> collecting tests now takes less than 2 seconds for all platforms. >>>>>>>>>>> >>>>>>>>>>> Impact to you: if your CL adds/deletes/renames web test cases, >>>>>>>>>>> you will be prompted. Follow the instructions there should be >>>>>>>>>>> enough. But >>>>>>>>>>> if you still have an issue, (even though we have tested many >>>>>>>>>>> different >>>>>>>>>>> scenarios we can think about), feel free to ping/email me. >>>>>>>>>>> >>>>>>>>>>> There is no change to how you run rwt locally. We added a new >>>>>>>>>>> switch to tell the swarming bots to load tests from the json file. >>>>>>>>>>> This >>>>>>>>>>> switch is turned off by default. Developers usually don't need to >>>>>>>>>>> turn this >>>>>>>>>>> on, unless all of your tests are saved to the json file and you are >>>>>>>>>>> running >>>>>>>>>>> a full test so want to save about 30 seconds. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> FWIW, I think this is an important and probably worthwhile change >>>>>>>>>> (disclaimer: I did review it and was involved in the design). The >>>>>>>>>> number of >>>>>>>>>> tests we have now is substantial and the startup delay is a >>>>>>>>>> significant >>>>>>>>>> cost. We need to be looking into ways to reduce this. >>>>>>>>>> >>>>>>>>>> I think it's possible we should turn this on by default and/or do >>>>>>>>>> some hooking to make things aware of when you've added new tests >>>>>>>>>> (e.g., via >>>>>>>>>> git status) to try and make this a little more seamless, but I also >>>>>>>>>> think >>>>>>>>>> landing it off by default was a good first step. >>>>>>>>>> >>>>>>>>>> Some may ask if we can just generate this at compile time. >>>>>>>>>> Unfortunately, no, we can't, because there's no way to keep it >>>>>>>>>> correctly >>>>>>>>>> up-to-date when you do add tests after having generated the file at >>>>>>>>>> least >>>>>>>>>> once [ we asked this ourselves and actually tried this approach and >>>>>>>>>> broke >>>>>>>>>> deterministic builds :( ]. >>>>>>>>>> >>>>>>>>>> I'm happy to hear other thoughts. Perhaps there are other ways we >>>>>>>>>> can optimize things to be fast enough ... >>>>>>>>>> >>>>>>>>>> -- Dirk >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Thanks for your time! >>>>>>>>>>> >>>>>>>>>>> Cheers, Weizhong >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> You received this message because you are subscribed to the >>>>>>>>>>> Google Groups "blink-dev" group. >>>>>>>>>>> To unsubscribe from this group and stop receiving emails from >>>>>>>>>>> it, send an email to [email protected]. >>>>>>>>>>> To view this discussion on the web visit >>>>>>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADXrSiprRWy_UCC0w2oohbSGGTbeP%3DdkyYnBOx7j8Y-abDSe2A%40mail.gmail.com >>>>>>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADXrSiprRWy_UCC0w2oohbSGGTbeP%3DdkyYnBOx7j8Y-abDSe2A%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>>>>>> . >>>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> You received this message because you are subscribed to the >>>>>>>>>> Google Groups "blink-dev" group. >>>>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>>>> send an email to [email protected]. >>>>>>>>>> To view this discussion on the web visit >>>>>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAEoffTAc389E6wteVXUAuKX8ZBFWBa2UEMwAbd9oCxJ9pxNCKQ%40mail.gmail.com >>>>>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAEoffTAc389E6wteVXUAuKX8ZBFWBa2UEMwAbd9oCxJ9pxNCKQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>>>>> . >>>>>>>>>> >>>>>>>>> -- >>>>>>> You received this message because you are subscribed to the Google >>>>>>> Groups "blink-dev" group. >>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>> send an email to [email protected]. >>>>>>> To view this discussion on the web visit >>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADXrSipBCHvjwf-AA1DGHUVjVzKQka5bBvTBnSRx5sEUpnJN_Q%40mail.gmail.com >>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADXrSipBCHvjwf-AA1DGHUVjVzKQka5bBvTBnSRx5sEUpnJN_Q%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>> . >>>>>>> >>>>>> -- >>>>>> You received this message because you are subscribed to the Google >>>>>> Groups "blink-dev" group. >>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>> send an email to [email protected]. >>>>>> To view this discussion on the web visit >>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAM0wra9B-aZTjoovP1WGDL80od64CtEL30jM8ch-2Pm_cd9Log%40mail.gmail.com >>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAM0wra9B-aZTjoovP1WGDL80od64CtEL30jM8ch-2Pm_cd9Log%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>> . >>>>>> >>>>> -- >>>> You received this message because you are subscribed to the Google >>>> Groups "blink-dev" group. >>>> To unsubscribe from this group and stop receiving emails from it, send >>>> an email to [email protected]. >>>> To view this discussion on the web visit >>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAEoffTA0pg5V92k-6gOORv5J%3DOiNVEuDH6VyW2tKTKjdd2uK%2BA%40mail.gmail.com >>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAEoffTA0pg5V92k-6gOORv5J%3DOiNVEuDH6VyW2tKTKjdd2uK%2BA%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>> . >>>> >>> -- You received this message because you are subscribed to the Google Groups "blink-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAEoffTCEfwWFjtz3oP8riELo2DdCJWTz9%2Bb2o1tpsK8LUY2x7g%40mail.gmail.com.
