Hey! This is an attempt to reland https://codereview.chromium.org/1492303003 which got reverted for (apparently unrelated) perf ...
4 years, 5 months ago
(2016-07-22 08:18:25 UTC)
#2
Hey!
This is an attempt to reland https://codereview.chromium.org/1492303003 which
got reverted for (apparently unrelated) perf regressions.
Can you gentlemen take a look?
Cheers,
Yoav
Charlie Harrison
Do you mind running the new page_cyclers with this change, just in case? kouhei@ has ...
4 years, 5 months ago
(2016-07-22 12:05:40 UTC)
#3
Do you mind running the new page_cyclers with this change, just in case?
kouhei@ has a CL where a config is added so that perf_bisect bots can run:
https://codereview.chromium.org/2137913002/
Adding kouhei@ to cc in case there is an easier way to do this now :)
Yoav Weiss
On 2016/07/22 12:05:40, csharrison wrote: > Do you mind running the new page_cyclers with this ...
4 years, 5 months ago
(2016-07-22 12:19:51 UTC)
#4
On 2016/07/22 12:05:40, csharrison wrote:
> Do you mind running the new page_cyclers with this change, just in case?
>
> kouhei@ has a CL where a config is added so that perf_bisect bots can run:
> https://codereview.chromium.org/2137913002/
> Adding kouhei@ to cc in case there is an easier way to do this now :)
added run-perf-test.cfg and currently trying to run "page_cycler_v2.typical_25".
Otherwise, DomDistillerJsTest is failing and not giving me much indication as to
why... Since it's a 70K lines JS test, I'm having trouble finding the issue
going over its code. Do you know who is familiar with that test and could help
point me in the right direction?
Yoav Weiss
On 2016/07/22 12:19:51, Yoav Weiss wrote: > On 2016/07/22 12:05:40, csharrison wrote: > > Do ...
4 years, 5 months ago
(2016-07-22 12:20:38 UTC)
#5
On 2016/07/22 12:19:51, Yoav Weiss wrote:
> On 2016/07/22 12:05:40, csharrison wrote:
> > Do you mind running the new page_cyclers with this change, just in case?
> >
> > kouhei@ has a CL where a config is added so that perf_bisect bots can run:
> > https://codereview.chromium.org/2137913002/
> > Adding kouhei@ to cc in case there is an easier way to do this now :)
>
> added run-perf-test.cfg and currently trying to run
"page_cycler_v2.typical_25".
Perf test running at https://codereview.chromium.org/2173793002
>
> Otherwise, DomDistillerJsTest is failing and not giving me much indication as
to
> why... Since it's a 70K lines JS test, I'm having trouble finding the issue
> going over its code. Do you know who is familiar with that test and could help
> point me in the right direction?
Charlie Harrison
On 2016/07/22 12:19:51, Yoav Weiss wrote: > On 2016/07/22 12:05:40, csharrison wrote: > > Do ...
4 years, 5 months ago
(2016-07-22 12:24:03 UTC)
#6
On 2016/07/22 12:19:51, Yoav Weiss wrote:
> On 2016/07/22 12:05:40, csharrison wrote:
> > Do you mind running the new page_cyclers with this change, just in case?
> >
> > kouhei@ has a CL where a config is added so that perf_bisect bots can run:
> > https://codereview.chromium.org/2137913002/
> > Adding kouhei@ to cc in case there is an easier way to do this now :)
>
> added run-perf-test.cfg and currently trying to run
"page_cycler_v2.typical_25".
>
> Otherwise, DomDistillerJsTest is failing and not giving me much indication as
to
> why... Since it's a 70K lines JS test, I'm having trouble finding the issue
> going over its code. Do you know who is familiar with that test and could help
> point me in the right direction?
My guess is as good as yours tbh. I think it is not in active development
unfortunately.
It looks like this could possibly be failing because the test waits until the
DOMContentLoaded event (I think). DCL doesn't wait for images afaik so it might
make sense to change that call to wait for onload. Not sure though.
Charlie Harrison
On 2016/07/22 12:24:03, csharrison wrote: > On 2016/07/22 12:19:51, Yoav Weiss wrote: > > On ...
4 years, 5 months ago
(2016-07-22 12:24:32 UTC)
#7
On 2016/07/22 12:24:03, csharrison wrote:
> On 2016/07/22 12:19:51, Yoav Weiss wrote:
> > On 2016/07/22 12:05:40, csharrison wrote:
> > > Do you mind running the new page_cyclers with this change, just in case?
> > >
> > > kouhei@ has a CL where a config is added so that perf_bisect bots can run:
> > > https://codereview.chromium.org/2137913002/
> > > Adding kouhei@ to cc in case there is an easier way to do this now :)
> >
> > added run-perf-test.cfg and currently trying to run
> "page_cycler_v2.typical_25".
> >
> > Otherwise, DomDistillerJsTest is failing and not giving me much indication
as
> to
> > why... Since it's a 70K lines JS test, I'm having trouble finding the issue
> > going over its code. Do you know who is familiar with that test and could
help
> > point me in the right direction?
>
> My guess is as good as yours tbh. I think it is not in active development
> unfortunately.
>
> It looks like this could possibly be failing because the test waits until the
> DOMContentLoaded event (I think). DCL doesn't wait for images afaik so it
might
> make sense to change that call to wait for onload. Not sure though.
Also, I don't see the perf bots running right now. I think you have to select
them manually in the codereview UI.
Yoav Weiss
CCing mdjones@ and nyquist@ as git blame points their way. Could you perhaps take a ...
4 years, 5 months ago
(2016-07-22 14:34:35 UTC)
#8
CCing mdjones@ and nyquist@ as git blame points their way. Could you perhaps
take a look and provide pointers to further debug why DomDistiller tests are
failing here?
4 years, 5 months ago
(2016-07-22 15:52:51 UTC)
#10
Adding wychen as well.
Yoav Weiss
On 2016/07/22 15:52:51, nyquist wrote: > Adding wychen as well. Thanks to nyquist@ who pointed ...
4 years, 5 months ago
(2016-07-22 16:13:49 UTC)
#11
On 2016/07/22 15:52:51, nyquist wrote:
> Adding wychen as well.
Thanks to nyquist@ who pointed me at the dom-distiller project, I think I found
the guilty party:
https://github.com/chromium/dom-distiller/blob/master/javatests/org/chromium/...
Seems like the project's tests are relying on current sync behavior :/
wychen@ - can you advise on the best course of action here?
pdr.
Still LGTM for the code change.
4 years, 5 months ago
(2016-07-22 16:28:02 UTC)
#12
Still LGTM for the code change.
wychen
On 2016/07/22 16:13:49, Yoav Weiss wrote: > wychen@ - can you advise on the best ...
4 years, 5 months ago
(2016-07-22 17:47:28 UTC)
#13
On 2016/07/22 16:13:49, Yoav Weiss wrote:
> wychen@ - can you advise on the best course of action here?
I think we can disable the tests that rely on the sync behavior
for now. The following CL can be referenced. The syntax of filter
is similar to gtest_filter.
https://codereview.chromium.org/1532453002
I'll find workarounds to revive these disabled tests later.
As for workarounds, based on my 5-minute reading, caching sounds
like a direction. However, if that's the case, I'd expect only
the first test would fail, but there were more than one failed
tests.
wychen
On 2016/07/22 17:47:28, wychen wrote: > On 2016/07/22 16:13:49, Yoav Weiss wrote: > > wychen@ ...
4 years, 5 months ago
(2016-07-22 18:26:37 UTC)
#14
On 2016/07/22 17:47:28, wychen wrote:
> On 2016/07/22 16:13:49, Yoav Weiss wrote:
> > wychen@ - can you advise on the best course of action here?
>
> I think we can disable the tests that rely on the sync behavior
> for now. The following CL can be referenced. The syntax of filter
> is similar to gtest_filter.
> https://codereview.chromium.org/1532453002
>
> I'll find workarounds to revive these disabled tests later.
>
> As for workarounds, based on my 5-minute reading, caching sounds
> like a direction. However, if that's the case, I'd expect only
> the first test would fail, but there were more than one failed
> tests.
Found a workaround. Warming the cache works, but it requires a
roll. I won't burden you with rolling, so you could just disable
them, and I'll handle the rest.
wychen
On 2016/07/22 18:26:37, wychen wrote: > On 2016/07/22 17:47:28, wychen wrote: > > On 2016/07/22 ...
4 years, 5 months ago
(2016-07-22 22:41:37 UTC)
#15
On 2016/07/22 18:26:37, wychen wrote:
> On 2016/07/22 17:47:28, wychen wrote:
> > On 2016/07/22 16:13:49, Yoav Weiss wrote:
> > > wychen@ - can you advise on the best course of action here?
> >
> > I think we can disable the tests that rely on the sync behavior
> > for now. The following CL can be referenced. The syntax of filter
> > is similar to gtest_filter.
> > https://codereview.chromium.org/1532453002
> >
> > I'll find workarounds to revive these disabled tests later.
> >
> > As for workarounds, based on my 5-minute reading, caching sounds
> > like a direction. However, if that's the case, I'd expect only
> > the first test would fail, but there were more than one failed
> > tests.
>
> Found a workaround. Warming the cache works, but it requires a
> roll. I won't burden you with rolling, so you could just disable
> them, and I'll handle the rest.
If you are not in a hurry to land this, the tests can pass after
https://codereview.chromium.org/2173883004/ lands.
Yoav Weiss
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
4 years, 4 months ago
(2016-07-25 08:53:24 UTC)
#16
The CQ bit was checked by yoav@yoav.ws to run a CQ dry run
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2173003002/60001
4 years, 4 months ago
(2016-07-25 08:53:38 UTC)
#17
4 years, 4 months ago
(2016-07-25 10:21:27 UTC)
#19
Dry run: This issue passed the CQ dry run.
Yoav Weiss
On 2016/07/22 22:41:37, wychen wrote: > On 2016/07/22 18:26:37, wychen wrote: > > On 2016/07/22 ...
4 years, 4 months ago
(2016-07-25 12:37:50 UTC)
#20
On 2016/07/22 22:41:37, wychen wrote:
> On 2016/07/22 18:26:37, wychen wrote:
> > On 2016/07/22 17:47:28, wychen wrote:
> > > On 2016/07/22 16:13:49, Yoav Weiss wrote:
> > > > wychen@ - can you advise on the best course of action here?
> > >
> > > I think we can disable the tests that rely on the sync behavior
> > > for now. The following CL can be referenced. The syntax of filter
> > > is similar to gtest_filter.
> > > https://codereview.chromium.org/1532453002
> > >
> > > I'll find workarounds to revive these disabled tests later.
> > >
> > > As for workarounds, based on my 5-minute reading, caching sounds
> > > like a direction. However, if that's the case, I'd expect only
> > > the first test would fail, but there were more than one failed
> > > tests.
> >
> > Found a workaround. Warming the cache works, but it requires a
> > roll. I won't burden you with rolling, so you could just disable
> > them, and I'll handle the rest.
>
> If you are not in a hurry to land this, the tests can pass after
> https://codereview.chromium.org/2173883004/ lands.
Thanks, wychan@! Tests now pass.
As the perf tests also pass, I'm landing this. Let's watch the perf bots to see
if they regress.
Yoav Weiss
The CQ bit was checked by yoav@yoav.ws
4 years, 4 months ago
(2016-07-25 12:37:59 UTC)
#21
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/224191)
4 years, 4 months ago
(2016-07-25 12:44:24 UTC)
#24
On 2016/07/25 13:22:13, csharrison wrote: > Don't commit the perf config. That's just for trybots. ...
4 years, 4 months ago
(2016-07-25 14:05:54 UTC)
#28
On 2016/07/25 13:22:13, csharrison wrote:
> Don't commit the perf config. That's just for trybots. The system is pretty
> confusing :(
of course *sigh*
Removed the perf config
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 4 months ago
(2016-07-25 15:09:05 UTC)
#29
Issue 2173003002: Load data URI images in an async way according to spec
(Closed)
Created 4 years, 5 months ago by Yoav Weiss
Modified 4 years, 4 months ago
Reviewers: pdr., Charlie Harrison, nyquist
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 0