On replacing WTF::ScopedMockClock by base::ScopedMockClockOverride
33 views
Skip to first unread message
Sergio Villar
Feb 28, 2019, 3:32:01 AM2/28/19
to platform-architecture-dev
Hi all,
as part of the OnionSoup effort I've lately been working on removing
wtf/time.h[1]. This involves several actions being one of them
replacing WTF::ScopedMockClock by something equivalent, being
base::ScopedMockClockOverride the apparent obvious choice.
However there are some difficulties and since there seem to be
different parallel ongoing efforts to achieve similar things in the
project, it seems a good idea to tackle this issue in this forum. In
particular I'd like to talk about two challenges.
1- USING MOCK CLOCKS IN MULTI-THREAD ENVIRONMENTS
I started working on the WTF::ScopedMockClock ->
base::ScopedMockClockOverridemigration migration here[2]. As you can
see the TSAN bot complains during the exit process of several unit
tests. The reason why it fails is because we're using
ScopedMockClockOverride under some conditions it was not made for.
Basically the mock clock is not created while in single-threaded (as
the docs specify). The access to the global pointers to the time
functions is not protected, and thus, there might be ongoing calls to
Time::Now() or TimeTicks::Now() while at the same time the test is
resetting the overrides to their original values.
The reason why there are multiple threads at the point the mock clock
is created, is because the blink test suite creates a task scheduler
during its initialization process. That task scheduler calls time
functions to check the length of tasks, to deal with timeouts...
That's the reason why Yuri Wiitala mentions here[3] that global time
functions should only be mocked very early in the process startup
routines and never change thereafter. Note that this approach would
mean that the mock clock should be able to go both forward and backward
in time, in order to make each test independent of the others run in
the same test suite.
I thought then about protecting the access to the global functions.
However I immediately realized that it would mean adding some overhead
to the production code for something that is only needed by tests. A
bit later I was told that Adithya Srinivasan had already started[4]
doing something like that previously. There are very interesting
comments raised by Yuri in that CL, summarizing a lot:
1) global overrides should only be mocked once and very early.
2) protecting global overrides adds overhead.
3) based on 1) and 2) we shouldn't design thread-safety for global
time overrides.
2- DISENTANGLING TEST CODE AND TEST RUNNER CODE
This is the other problem I've stumbled into while migrating the code.
It's already very well described in this doc[5] (Charlie Andrews hit
the same issues a while ago). There is code in TreadLoadTracker that
verifies that the task that was run (in this case a unit test) finished
before the current Now() timestamp. The problem here is that, the
measurement of the task length is done with the "real" time, i.e. with
no overrides, but the test which is using a mock clock can go arbitrary
forward on time (actually the current ScopedMockClockOveride starts
with a 1-year offset to avoid starting in 0 which is interpreted as
null).
If I am not wrong, mocking the clocks very early and while in single-
threaded won't completely fix this second issue, because although we
won't have inconsistencies in the measurements (as long as we don't go
back in time) we would have the problem of the test runner code
reporting about tasks taking arbitrary long periods of time to be
completed (although in "real" time it might be just some ms).
So what do you folks think? Is there anyone working on these issues?
Has somebody come up with a satisfactory solution/design for these
challenges? Any idea that would allow us to move forward?
BR
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=919383
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1477590
[3]
https://chromium-review.googlesource.com/c/chromium/src/+/1318585/13#message-9eef6f679bbb679af919acf7e2a295b6819055d8
[4] https://chromium-review.googlesource.com/c/chromium/src/+/1318585
[5]
https://docs.google.com/document/d/1Xjc67VhY-WCt4S2PXyDpyg-E7IZAZf3LRfHu31ttXUo
as part of the OnionSoup effort I've lately been working on removing
wtf/time.h[1]. This involves several actions being one of them
replacing WTF::ScopedMockClock by something equivalent, being
base::ScopedMockClockOverride the apparent obvious choice.
However there are some difficulties and since there seem to be
different parallel ongoing efforts to achieve similar things in the
project, it seems a good idea to tackle this issue in this forum. In
particular I'd like to talk about two challenges.
1- USING MOCK CLOCKS IN MULTI-THREAD ENVIRONMENTS
I started working on the WTF::ScopedMockClock ->
base::ScopedMockClockOverridemigration migration here[2]. As you can
see the TSAN bot complains during the exit process of several unit
tests. The reason why it fails is because we're using
ScopedMockClockOverride under some conditions it was not made for.
Basically the mock clock is not created while in single-threaded (as
the docs specify). The access to the global pointers to the time
functions is not protected, and thus, there might be ongoing calls to
Time::Now() or TimeTicks::Now() while at the same time the test is
resetting the overrides to their original values.
The reason why there are multiple threads at the point the mock clock
is created, is because the blink test suite creates a task scheduler
during its initialization process. That task scheduler calls time
functions to check the length of tasks, to deal with timeouts...
That's the reason why Yuri Wiitala mentions here[3] that global time
functions should only be mocked very early in the process startup
routines and never change thereafter. Note that this approach would
mean that the mock clock should be able to go both forward and backward
in time, in order to make each test independent of the others run in
the same test suite.
I thought then about protecting the access to the global functions.
However I immediately realized that it would mean adding some overhead
to the production code for something that is only needed by tests. A
bit later I was told that Adithya Srinivasan had already started[4]
doing something like that previously. There are very interesting
comments raised by Yuri in that CL, summarizing a lot:
1) global overrides should only be mocked once and very early.
2) protecting global overrides adds overhead.
3) based on 1) and 2) we shouldn't design thread-safety for global
time overrides.
2- DISENTANGLING TEST CODE AND TEST RUNNER CODE
This is the other problem I've stumbled into while migrating the code.
It's already very well described in this doc[5] (Charlie Andrews hit
the same issues a while ago). There is code in TreadLoadTracker that
verifies that the task that was run (in this case a unit test) finished
before the current Now() timestamp. The problem here is that, the
measurement of the task length is done with the "real" time, i.e. with
no overrides, but the test which is using a mock clock can go arbitrary
forward on time (actually the current ScopedMockClockOveride starts
with a 1-year offset to avoid starting in 0 which is interpreted as
null).
If I am not wrong, mocking the clocks very early and while in single-
threaded won't completely fix this second issue, because although we
won't have inconsistencies in the measurements (as long as we don't go
back in time) we would have the problem of the test runner code
reporting about tasks taking arbitrary long periods of time to be
completed (although in "real" time it might be just some ms).
So what do you folks think? Is there anyone working on these issues?
Has somebody come up with a satisfactory solution/design for these
challenges? Any idea that would allow us to move forward?
BR
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=919383
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1477590
[3]
https://chromium-review.googlesource.com/c/chromium/src/+/1318585/13#message-9eef6f679bbb679af919acf7e2a295b6819055d8
[4] https://chromium-review.googlesource.com/c/chromium/src/+/1318585
[5]
https://docs.google.com/document/d/1Xjc67VhY-WCt4S2PXyDpyg-E7IZAZf3LRfHu31ttXUo
Jeremy Roman
Feb 28, 2019, 2:54:37 PM2/28/19
to Sergio Villar, platform-architecture-dev
I didn't work directly on this issue, but my impression had been that we would likely need some capacity to make the few things that do outlive a single test (like the task scheduler) explicitly aware that they are running in test and allow the test runner to reset their state in between. If we managed that, then as long as no individual test case "goes back in time" the the task scheduler shouldn't observe that, either. It might be that something actually depends on time passing (where a test does not do so), which would complicate things a little further.
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/7c8172c6-9c39-4234-a65e-0418e4160ab1%40chromium.org.
Yuri Wiitala
Feb 28, 2019, 4:46:15 PM2/28/19
to Jeremy Roman, Sergio Villar, platform-architecture-dev
Thanks, Sergio, for putting together a nicely-written problem statement for the wider audience.
w.r.t. changing the mock clock between tests: Are the individual tests running in the same process? I thought gtest forked a separate process for each one? This would mean that the mocked time values between tests won't end up mixing in the task runner.
A couple of random ideas:
1a. Instead of scoped mocking, only allow one override that must remain in-place permanently for the life of the process. Then...
1b. Force the mock clock to always start at the prior clock's Now() point.
2. Add some infrastructure to notify the task scheduler that the clock is being changed. (Or, maybe just have some way to cancel all tasks from all task queues, and ScopedMockClockOverrides could call that.) The threading around this might be very tricky, though.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CACuR13ecqD0zEDaN%2B9b2MPBmM7c_iw-absN69F1-bGTc0ssaTQ%40mail.gmail.com.
Jeremy Roman
Feb 28, 2019, 5:04:51 PM2/28/19
to Yuri Wiitala, Sergio Villar, platform-architecture-dev
On Thu, Feb 28, 2019 at 4:46 PM Yuri Wiitala <m...@chromium.org> wrote:
Thanks, Sergio, for putting together a nicely-written problem statement for the wider audience.w.r.t. changing the mock clock between tests: Are the individual tests running in the same process? I thought gtest forked a separate process for each one?
No, it doesn't. It divides the test cases among some number of processes between 1 and the number of tests. It is possible for state from one unit test to affect another if it is not cleaned up properly (this mostly affects globals). When that happens, the test will often pass on retry.
Jeremy Roman
Feb 28, 2019, 5:17:24 PM2/28/19
to Yuri Wiitala, Sergio Villar, platform-architecture-dev
On Thu, Feb 28, 2019 at 5:04 PM Jeremy Roman <jbr...@chromium.org> wrote:
On Thu, Feb 28, 2019 at 4:46 PM Yuri Wiitala <m...@chromium.org> wrote:Thanks, Sergio, for putting together a nicely-written problem statement for the wider audience.w.r.t. changing the mock clock between tests: Are the individual tests running in the same process? I thought gtest forked a separate process for each one?No, it doesn't. It divides the test cases among some number of processes between 1 and the number of tests. It is possible for state from one unit test to affect another if it is not cleaned up properly (this mostly affects globals). When that happens, the test will often pass on retry.This would mean that the mocked time values between tests won't end up mixing in the task runner.A couple of random ideas:1a. Instead of scoped mocking, only allow one override that must remain in-place permanently for the life of the process. Then...1b. Force the mock clock to always start at the prior clock's Now() point.
Yeah, this is similar to ideas that Adithya had. It seems like we can:
- in test processes, replace the clock function with one that's mockable
- either reset all state between tests or make this mock clock function only support advancing (either with real time given some offset, or explicitly under test control)
Pseudocode for that clock function (used only in test binaries):
TimeTicks Now() {
AutoLock lock(mutex_);
if (mode_ == kRealTime) {
return offset_ + RealNow();
} else {
return offset_;
}
}
void SwitchToRealTime() {
AutoLock lock(mutex_);
offset_ -= RealNow();
mode_ = kRealTime;
}
void SwitchToControlledTime() {
AutoLock lock(mutex_);
offset_ += RealNow();
mode_ = kControlledTime;
}
void AdvanceTime(TimeDelta delta) {
AutoLock lock(mutex_);
DCHECK_EQ(mode_, kControlledTime);
offset_ += delta;
}
class ControlledTimeScope {
public:
ControlledTimeScope() { SwitchToControlledTime(); }
~ControlledTimeScope() { SwitchToRealTime(); } // possibly add'l boring complexity to allow nested scopes
};
That seems to suffice as long as we don't have some need for the task scheduler to see time as advancing even when the time source is controlled by test.
2. Add some infrastructure to notify the task scheduler that the clock is being changed. (Or, maybe just have some way to cancel all tasks from all task queues, and ScopedMockClockOverrides could call that.) The threading around this might be very tricky, though.
Yup, this is what I meant by "[allowing] the test runner to reset their state".
Sergio Villar Senin
Mar 5, 2019, 11:23:30 AM3/5/19
to platform-arc...@chromium.org
referring to the 3 clock global function pointers?
So assuming that we create a scoped class to handle mock clock inside
the tests, how would tests have access to that clock in order to
advance it?. Would it be a global exposed to tests? What should be the
best location for it?
BR
Jeremy Roman
Mar 5, 2019, 4:04:04 PM3/5/19
to Sergio Villar Senin, platform-architecture-dev
Yes.
So assuming that we create a scoped class to handle mock clock inside
the tests, how would tests have access to that clock in order to
advance it?. Would it be a global exposed to tests? What should be the
best location for it?
Just functions (like the above but possibly with more descriptive names, and expanded to deal with the three kinds of time) ought to suffice, probably in base:: or base::test::, and living either in base/test/ or base/time/. You and/or your reviewer might do it slightly differently than I imagined; that's fine too. :)
BR
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/a5b8d37c045d5f4486c72d67b5f78ea39ce92498.camel%40igalia.com.
Sergio Villar Senin
Mar 8, 2019, 6:35:41 AM3/8/19
to platform-arc...@chromium.org
O Xov, 28-02-2019 ás 00:32 -0800, Sergio Villar escribiu:
> Hi all,
>
> as part of the OnionSoup effort I've lately been working on removing
> wtf/time.h[1]. This involves several actions being one of them
> replacing WTF::ScopedMockClock by something equivalent, being
> base::ScopedMockClockOverride the apparent obvious choice.
I was thinking a bit about it and what if replacing them with
> Hi all,
>
> as part of the OnionSoup effort I've lately been working on removing
> wtf/time.h[1]. This involves several actions being one of them
> replacing WTF::ScopedMockClock by something equivalent, being
> base::ScopedMockClockOverride the apparent obvious choice.
base::ScopedMockClockOverride is not the obvious choice for all the
cases?
I think we might consider, for simple test cases, to forget about
overriding time (which is dangerous) and use instead
TestMockTimeTaskRunner. I understand that this might involve adding
SetClockForTesting() to some classes and using clock.Now() instead of
base::Time::Now() inside them. There are not many tests using
WTF::ScopedMockClock after all.
However there is a potential blocker which is web_view_test.cc which
triggers all the page load mechanisms. I haven't researched about it in
deep but migrating it to a mock task runner would likely involve
changing a lot of classes to make them work with clocks instead of
directly calling WTF::Time* and base::Time* functions.
In any case, I understand we are not in a hurry so I think it might
make sense to devote some time to this line of work before permanently
force the tests to always run in a time overriden environment.
Note that since we're planning to early override the time functions and
never revert that during process lifetime the practical difference
would be minimal but by using clocks we might have some more
flexibility and we might even think about completely removing the
overrides.
What do you folks think?
BR
Jeremy Roman
Mar 8, 2019, 4:31:07 PM3/8/19
to Sergio Villar Senin, platform-architecture-dev
It sounds nice, but my intuition is that we have a sufficiently large number of places that time is used that this will prove difficult (and some places may not have a convenient place to store a TickClock*, especially without using significant more memory in production in the case of small-but-numerous objects). If it is viable, it's the conceptually cleanest approach.
BR
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/2187ca68fd3e01801490bd06b8cb0e061b896c74.camel%40igalia.com.
Alex Clarke
Mar 11, 2019, 6:17:55 AM3/11/19
to Jeremy Roman, Sergio Villar Senin, platform-architecture-dev
I'm pretty sure this is correct :(
It would be great if we could reliably shut down everything but the main thread in tests. I'm of the opinion it would be good to built the blink equivalent of TestBrowserThreadBundle which would provide a uniform way of writing tests with mock time and solve all these TSAN issues.
--BR
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/2187ca68fd3e01801490bd06b8cb0e061b896c74.camel%40igalia.com.
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CACuR13e4OLsw3_xXTOP7fc1aXXR6BNMb1oAzj8BQmQ%2B%2BS3wRXw%40mail.gmail.com.
Sergio Villar Senin
Mar 11, 2019, 5:22:50 PM3/11/19
to platform-arc...@chromium.org
O Lun, 11-03-2019 ás 10:17 +0000, 'Alex Clarke' via platform-
architecture-dev escribiu:
So I gave it a try and I managed to port all the test cases to
TestMockTimeRunner even the ones from WebViewTest. The changes are
pretty small and good loooking IMO (I'll upload the CL soon).
There is just 1 of them failing which is
WebViewTest.InputDelayReported. Actually only the
"PageLoad.InteractiveTiming.InputTimestamp2" histogram checks fail. The
reason is pretty simple, the DocumentLoader is passing the
InteractiveDetector a value for the navigation start in "real" ticks
instead of the mock ones. For the rest of the test cases in the other
classes it was pretty easy to add a SetTickClockForTesting() but for
the WebViewTest one I was wondering whether that makes sense at all as
it would involve a lot of changes.
What do you think about adding an add-hoc fix for those particular
checks instead of modifying the whole doc loader? I was thinking about
modifying the last 3 checks so that instead of having, i.e.
histogram_tester.ExpectBucketCount(
"PageLoad.InteractiveTiming.InputTimestamp2", 70, 1);
we would have something like
histogram_tester.ExpectBucketCount(
"PageLoad.InteractiveTiming.InputTimestamp2",
delta_between_real_and_mock_clock + 70, 1);
with obviously some comment explaining that we're doing this in order
to avoid migrating the whole DocumentLoader hierarchy and related
classes.
BR
architecture-dev escribiu:
TestMockTimeRunner even the ones from WebViewTest. The changes are
pretty small and good loooking IMO (I'll upload the CL soon).
There is just 1 of them failing which is
WebViewTest.InputDelayReported. Actually only the
"PageLoad.InteractiveTiming.InputTimestamp2" histogram checks fail. The
reason is pretty simple, the DocumentLoader is passing the
InteractiveDetector a value for the navigation start in "real" ticks
instead of the mock ones. For the rest of the test cases in the other
classes it was pretty easy to add a SetTickClockForTesting() but for
the WebViewTest one I was wondering whether that makes sense at all as
it would involve a lot of changes.
What do you think about adding an add-hoc fix for those particular
checks instead of modifying the whole doc loader? I was thinking about
modifying the last 3 checks so that instead of having, i.e.
histogram_tester.ExpectBucketCount(
"PageLoad.InteractiveTiming.InputTimestamp2", 70, 1);
we would have something like
histogram_tester.ExpectBucketCount(
"PageLoad.InteractiveTiming.InputTimestamp2",
delta_between_real_and_mock_clock + 70, 1);
with obviously some comment explaining that we're doing this in order
to avoid migrating the whole DocumentLoader hierarchy and related
classes.
BR
Jeremy Roman
Mar 11, 2019, 6:27:04 PM3/11/19
to Sergio Villar Senin, platform-architecture-dev
That sounds like it could be tolerable, but hard to tell without knowing details.
BR
> It would be great if we could reliably shut down everything but the
> main thread in tests. I'm of the opinion it would be good to built
> the blink equivalent of TestBrowserThreadBundle which would provide a
> uniform way of writing tests with mock time and solve all these TSAN
> issues.
>
>
>
> >
> > > BR
> > >
> > > --
> > > You received this message because you are subscribed to the
> > > Google Groups "platform-architecture-dev" group.
> > > To unsubscribe from this group and stop receiving emails from it,
> > > send an email to
> > > platform-architect...@chromium.org.
> > > To post to this group, send email to
> > > platform-arc...@chromium.org.
> > > To view this discussion on the web visit
> > > https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/2187ca68fd3e01801490bd06b8cb0e061b896c74.camel%40igalia.com
> > > .
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/1df428642b0e76e6798dc32f7e5dc38dc5a0d978.camel%40igalia.com.
Sergio Villar Senin
May 2, 2019, 10:57:27 AM5/2/19
to Jeremy Roman, platform-architecture-dev
O Ven, 08-03-2019 ás 16:30 -0500, Jeremy Roman escribiu:
> > Note that since we're planning to early override the time functions
> > and
> > never revert that during process lifetime the practical difference
> > would be minimal but by using clocks we might have some more
> > flexibility and we might even think about completely removing the
> > overrides.
> >
> > What do you folks think?
>
> It sounds nice, but my intuition is that we have a sufficiently large
> number of places that time is used that this will prove difficult
> (and some places may not have a convenient place to store a
> TickClock*, especially without using significant more memory in
> production in the case of small-but-numerous objects). If it
> is viable, it's the conceptually cleanest approach.
So after a lot of work adapting tests and several classes I managed to
> > Note that since we're planning to early override the time functions
> > and
> > never revert that during process lifetime the practical difference
> > would be minimal but by using clocks we might have some more
> > flexibility and we might even think about completely removing the
> > overrides.
> >
> > What do you folks think?
>
> It sounds nice, but my intuition is that we have a sufficiently large
> number of places that time is used that this will prove difficult
> (and some places may not have a convenient place to store a
> TickClock*, especially without using significant more memory in
> production in the case of small-but-numerous objects). If it
> is viable, it's the conceptually cleanest approach.
completely remove all the usages of SetTimeFunctionsForTesting() from
the tree. Apart from tests, several classes had to be ported as well so
that they use a base::Clock/base::TickClock objects instead of the
utility functions defined in time.h.
Those clocks can be overridden by custom ones
(Set{Tick}ClockForTesting) in tests. I tried hard to store the pointers
to the clocks in the individual instances, but there are a few cases
which use a global pointer to the testing clocks instead (as in many
other cases in the tree) in order not to change way too many related
classes in the hierarchy.
I was asked to migrate everything to prove that the approach was good,
so this means that now we have 8 dependent CLs to be reviewed in case
you folks are OK with the path I followed. As there are a lot of
changes involved I thought it'd be nice to send an email to the list
for both a heads-up and also to provide an eagle-eye view of the whole
migration.
The approach I followed was:
1. Migrate ScopedMockClock to TestMockTimeTaskRunner
2. Migrate TestingPlatformSupportWithMockScheduler to
TestMockTimeTaskRunner
3. Remove ScopedMockClock
4. Remove SetTimeFunctionsForTesting
This is the hierarchy of CLs to be reviewed:
1518518 - ScopedMockClock->TestMockTimeTaskRunner initial migrations
|-> 1593252 - Additional migrations of code added later
|-> 1590101 - DOMTimer & WindowPerformance migrations
|-> 1588650 - WebViewTest migration
|-> 1590103 - MemoryInfoTest migration
|-> 1591634 - TestingPlatformSupportWithMockScheduler migration
|-> 1593257 - Remove ScopedMockClock from tree
|-> 1593372 - Remove SetTimeFunctionsForTesting
Anyone brave enough to glimpse at the whole thing and to at least start
reviewing the initial CLs? I'll add the suggested OWNERs anyway.
BR
Kentaro Hara
May 2, 2019, 12:21:22 PM5/2/19
to Sergio Villar Senin, Jeremy Roman, platform-architecture-dev
I'm mostly delegating the review of this series of CLs to Jeremy but want to say a big thank you! Removing wtf/time.h is a huge simplification for the Blink code base :)
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/d2109cbd5110183a39d6228d200b7c93da865e07.camel%40igalia.com.
Kentaro Hara, Tokyo, Japan
Message has been deleted
g...@chromium.org
May 7, 2019, 2:15:30 PM5/7/19
to platform-architecture-dev, svi...@igalia.com, jbr...@chromium.org
Hello all, I was just made aware of this discussion.
This is precisely the problem that base::test::ScopedTaskEnvironment aims to solve (mock time + manage the thread pool (i.e. former "task scheduler") between individual tests).
It currently doesn't support multi-threaded mock-time and mocking Now() is supported but bound to main thread's fast forward logic at the moment (ScopedTaskEnvironment::NowSource::MAIN_THREAD_MOCK_TIME).
The ultimate goal is to support multi-threaded mock-time and I'm happy to find ways to prioritize this if it helps unblock Blink.
At BlinkOn I was made aware that ScopedTaskEnvironment isn't quite suitable for Blink at the moment because we don't tear down RenderThreadImpl between each tests and thus can't have a "scoped" environment.
I think we should fix this. As alexclarke@ mentioned above, ScopedTaskEnvironment can be overridden to provide additional elements. We do this already in the browser code for content::TestBrowserThreadBundle which is a ScopedTaskEnvironment that also enables BrowserThread APIs. We should have a Blink equivalent that sets up a Blink main thread (if it must use a non-reset global RenderThreadImpl internally at the beginning so be it, but at least it gives us a point in the single-threaded phases of setup/teardown where we can do things like mock Now()).
I think that long-term this will be far less invasive than plumbing SetTickClockForTesting() and SetTaskRunnerForTesting() everywhere.
Lastly, re. TestMockTimeTaskRunner : on the browser-side it is close to deprecated as ScopedTaskEnvironment is a strictly superior construct. The only valid use case remaining is having multiple TestMockTimeTaskRunner to simulate lock step threading from the main test thread but this is rare. I think we should aim to be consistent with this direction in Blink.
Cheers,
Gab
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architecture-dev+unsub...@chromium.org.
To post to this group, send email to platform-architecture-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/d2109cbd5110183a39d6228d200b7c93da865e07.camel%40igalia.com.
Kentaro Hara
May 7, 2019, 9:39:26 PM5/7/19
to Gabriel Charette, platform-architecture-dev, Sergio Villar, Jeremy Roman
Thanks gab for the input!
The direction sounds reasonable to me but I'm a bit concerned that it will be a huge amount of work.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/d2109cbd5110183a39d6228d200b7c93da865e07.camel%40igalia.com.
--Kentaro Hara, Tokyo, Japan
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/c436ad9d-b2ec-4781-bfd5-eb13c1528468%40chromium.org.
Sergio Villar Senin
May 8, 2019, 3:54:35 AM5/8/19
to g...@chromium.org, platform-architecture-dev, jbr...@chromium.org
O Mar, 07-05-2019 ás 11:15 -0700, g...@chromium.org escribiu:
> Hello all, I was just made aware of this discussion.
>
> This is precisely the problem
> that base::test::ScopedTaskEnvironment aims to solve (mock time +
> manage the thread pool (i.e. former "task scheduler") between
> individual tests).
> It currently doesn't support multi-threaded mock-time and mocking
> Now() is supported but bound to main thread's fast forward logic at
> the moment (ScopedTaskEnvironment::NowSource::MAIN_THREAD_MOCK_TIME).
> The ultimate goal is to support multi-threaded mock-time and I'm
> happy to find ways to prioritize this if it helps unblock Blink.
>
> At BlinkOn I was made aware that ScopedTaskEnvironment isn't quite
> suitable for Blink at the moment because we don't tear down
> RenderThreadImpl between each tests and thus can't have a "scoped"
> environment.
Ah nice to know, I wasn't aware of that either.
> Hello all, I was just made aware of this discussion.
>
> This is precisely the problem
> that base::test::ScopedTaskEnvironment aims to solve (mock time +
> manage the thread pool (i.e. former "task scheduler") between
> individual tests).
> It currently doesn't support multi-threaded mock-time and mocking
> Now() is supported but bound to main thread's fast forward logic at
> the moment (ScopedTaskEnvironment::NowSource::MAIN_THREAD_MOCK_TIME).
> The ultimate goal is to support multi-threaded mock-time and I'm
> happy to find ways to prioritize this if it helps unblock Blink.
>
> At BlinkOn I was made aware that ScopedTaskEnvironment isn't quite
> suitable for Blink at the moment because we don't tear down
> RenderThreadImpl between each tests and thus can't have a "scoped"
> environment.
> I think we should fix this. As alexclarke@ mentioned above,
> ScopedTaskEnvironment can be overridden to provide additional
> elements. We do this already in the browser code for
> content::TestBrowserThreadBundle which is a ScopedTaskEnvironment
> that also enables BrowserThread APIs. We should have a Blink
> equivalent that sets up a Blink main thread (if it must use a non-
> reset global RenderThreadImpl internally at the beginning so be it,
> but at least it gives us a point in the single-threaded phases of
> setup/teardown where we can do things like mock Now()).
> I think that long-term this will be far less invasive than plumbing
> SetTickClockForTesting() and SetTaskRunnerForTesting() everywhere.
in context. Completely removing the dependency of Blink's wtf/time.h
would mean migrating <20 tests out of 13k. Knowing that
CurrentTimeTicks() & family are basically everywhere in Blink's
codebase I think it's a pretty good deal, not ideal, but fair enough.
> Lastly, re. TestMockTimeTaskRunner : on the browser-side it is close
> to deprecated as ScopedTaskEnvironment is a strictly superior
> construct. The only valid use case remaining is having multiple
> TestMockTimeTaskRunner to simulate lock step threading from the main
> test thread but this is rare. I think we should aim to be consistent
> with this direction in Blink.
this, I noticed that there are many different ways to mock time in
Chromium right now. Deciding which one was the most suitable took a lot
of time because all of them have cons and pros. You were on top of
ScopedTaskEnvironment so you're aware of the differences with previous
approaches but for people writing new tests is far from evident. IMO we
should focus as well on not duplicating the way of mocking time. And
IMO the approach I followed goes in the right direction, even though
the long-term solution is likely the one you pointed out.
BR
> > > To post to this group, send email to
> > > platform-arc...@chromium.org.
Gabriel Charette
May 8, 2019, 10:26:19 AM5/8/19
to Sergio Villar Senin, Alex Clarke, Gabriel Charette, platform-architecture-dev, Jeremy Roman, Alexander Timin, Sami Kyostila
> So, what's your proposal...? Are you proposing deferring this work until Blink rewrites tests to use ScopedTaskEnvironment?
I'm proposing having a subclass of ScopedTaskEnvironment for Blink. e.g. ScopedBlinkTaskEnvironment. Then tests can be migrated to use that instead of migrating product code to use TickClock (not *all* tests initially, just the ones in the scope of the wtf/time.h migration). The amount of work is, I think, similar to plumbing TickClock everywhere but it's less intrusive and it gets Blink aligned with the rest of the codebase (so we don't need a second migration later). Re. migrating off wtf/time.h; we could have ScopedBlinkTaskEnvironment also mock time in wtf/time.h while code is migrated off of it? As such, I don't think this is additional work but rather a direct step to where we want to be ultimately.
AFAIK there aren't short-term plans. +Alex Clarke also wanted it to happen but I don't think he's actively working on it.
> I think that long-term this will be far less invasive than plumbing
> SetTickClockForTesting() and SetTaskRunnerForTesting() everywhere.
While I agree that is less invasive I think we should also put things
in context. Completely removing the dependency of Blink's wtf/time.h
would mean migrating <20 tests out of 13k. Knowing that
CurrentTimeTicks() & family are basically everywhere in Blink's
codebase I think it's a pretty good deal, not ideal, but fair enough.
Isn't it the same amount of work? Either migrate to ScopedBlinkTaskEnvironment or to MockTickClock (and in the latter case we have to plumb TickClocks everywhere).
> Lastly, re. TestMockTimeTaskRunner : on the browser-side it is close
> to deprecated as ScopedTaskEnvironment is a strictly superior
> construct. The only valid use case remaining is having multiple
> TestMockTimeTaskRunner to simulate lock step threading from the main
> test thread but this is rare. I think we should aim to be consistent
> with this direction in Blink.
I think this is a quite important comment. When I started working on
this, I noticed that there are many different ways to mock time in
Chromium right now. Deciding which one was the most suitable took a lot
of time because all of them have cons and pros. You were on top of
ScopedTaskEnvironment so you're aware of the differences with previous
approaches but for people writing new tests is far from evident. IMO we
should focus as well on not duplicating the way of mocking time. And
IMO the approach I followed goes in the right direction, even though
the long-term solution is likely the one you pointed out.
Right, we need to rectify this. ScopedTaskEnvironment was introduced in 2017 to solve this problem (and converge all test task runners to it). But it's a part time project and it's not yet feature complete (want multi-threaded mock-time; need to support Blink; need to support browser_tests). As such we haven't yet made the step to explicitly flag classes such as TestMockTimeTaskRunner as deprecated in favor of ScopedTaskEnvironment.
It's close enough to feature complete now that I'm happy to prioritize the last stretch.
Alex Clarke
May 9, 2019, 5:49:31 AM5/9/19
to Gabriel Charette, Alexander Timin, Sergio Villar Senin, Alex Clarke, platform-architecture-dev, Jeremy Roman, Alexander Timin, Sami Kyostila
From: Gabriel Charette <g...@chromium.org>
Date: Wed, 8 May 2019 at 15:26
To: Sergio Villar Senin, Alex Clarke
Cc: Gabriel Charette, platform-architecture-dev, Jeremy Roman, Alexander Timin, Sami Kyostila
To: Sergio Villar Senin, Alex Clarke
Cc: Gabriel Charette, platform-architecture-dev, Jeremy Roman, Alexander Timin, Sami Kyostila
I'm not planning to work on this currently, but I understand +Alexander Timin was thinking about doing so.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAJTZ7L%2BvPRm-Dq1mX%3DenVx-b1nZeZ1bmceAtuLPLepoft82mug%40mail.gmail.com.
Sergio Villar Senin
May 9, 2019, 6:12:18 AM5/9/19
to platform-arc...@chromium.org
O Mér, 08-05-2019 ás 10:26 -0400, Gabriel Charette escribiu:
> > > I think that long-term this will be far less invasive than
> > plumbing
> > > SetTickClockForTesting() and SetTaskRunnerForTesting()
> > everywhere.
> >
> > While I agree that is less invasive I think we should also put
> > things
> > in context. Completely removing the dependency of Blink's
> > wtf/time.h
> > would mean migrating <20 tests out of 13k. Knowing that
> > CurrentTimeTicks() & family are basically everywhere in Blink's
> > codebase I think it's a pretty good deal, not ideal, but fair
> > enough.
>
> Isn't it the same amount of work? Either migrate to
> ScopedBlinkTaskEnvironment or to MockTickClock (and in the latter
> case we have to plumb TickClocks everywhere).
It's likely the same amount of work indeed. The thing is that the
> > > I think that long-term this will be far less invasive than
> > plumbing
> > > SetTickClockForTesting() and SetTaskRunnerForTesting()
> > everywhere.
> >
> > While I agree that is less invasive I think we should also put
> > things
> > in context. Completely removing the dependency of Blink's
> > wtf/time.h
> > would mean migrating <20 tests out of 13k. Knowing that
> > CurrentTimeTicks() & family are basically everywhere in Blink's
> > codebase I think it's a pretty good deal, not ideal, but fair
> > enough.
>
> Isn't it the same amount of work? Either migrate to
> ScopedBlinkTaskEnvironment or to MockTickClock (and in the latter
> case we have to plumb TickClocks everywhere).
migration to TestMockTimeTaskRunner+TickClocks is already completed, a
bunch of dependent CLs have been already uploaded and we have lgtm for
all of them.
So following now the ScopedTaskEnvironment approach would actually mean
x2 the work since we'd have to throw all the TestMockTimeTaskRunner to
the trash.
Don't misunderstand me, I don't think we are in a hurry and we should
indeed aim for the "right" solution. I just wanted to state that we
already have a solution (perhaps not the best one) on the one hand and
in the other we only have a plan with apparently no assignee to carry
it on.
BR
Alexander Timin
May 9, 2019, 9:22:04 AM5/9/19
to Sergio Villar Senin, platform-architecture-dev
From the blink scheduler perspective +1 to land these things, it would bring us closer to having ScopedTaskEnvironment in blink.
One comment that I have if that I would prefer us to pass base::TickClock in the constructor of the object (with base::DefaultClock::GetInstance) as a default and making it const rather than having a SetClock method (feel free to disagree with me here).
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/522cee796d534522365bd8304e9689574e07b3c1.camel%40igalia.com.
Gabriel Charette
May 14, 2019, 4:35:09 PM5/14/19
to Alexander Timin, Sergio Villar Senin, platform-architecture-dev
SGTM, thanks Alexander for chiming in.
I hadn't realized the work had already been done (hence why I was saying it would be the same amount of work to do it the way that doesn't imply a migration in the future).
Sorry for stirring the soup; I only wanted to highlight what the ideal end state is and what we already have on the non-WTF side. If blink owners see this as an acceptable intermediate path to keep us moving on an already solid cleanup then let's do it.
Thanks,
Gab
You received this message because you are subscribed to a topic in the Google Groups "platform-architecture-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/0x9xaxlBvB4/unsubscribe.
To unsubscribe from this group and all its topics, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CALHg4nk%2B4xgoKGRSRU-MoJNhk9EMEApJM%3DEejQ6J%2BrFk2S2S-w%40mail.gmail.com.
Reply all
Reply to author
Forward
0 new messages