two cpu profiler tests are flaky on windows.

two cpu profiler tests are flaky on windows.

BUG=none
TEST=LogExistingFunctionSourceURLCheck, SourceURLSupportForNewFunctions
TBR=yurys

Committed: https://code.google.com/p/v8/source/detail?r=15088

[loislo, 2013-06-12 14:34:33]

Committed patchset #1 manually as r15088 (presubmit successful).

[Sven Panne, 2013-06-13 06:27:08]

I actually don't like this change, for several reasons:

Bumping up some arbitrary limits is not a solution at all, the flakiness already
came back (see our bots). Note that those tests regularly fail on our bots for
months now, with more than half a dozen half-hearted repair attempts. This is
really annoying for the whole team, because you constantly get "you broke the
build" mails for no good reason.

The other reason why I don't like the change is that the tests are not "good
team players" in the unit test sense: Any unit test taking longer than 100ms
(probably even less) is very bad if you consider the runtime of the whole suite.
If every test took 500ms and we would be able to perfectly distribute this on 32
cores, the suite would take 2min35sec, on a 4 core Laptop almost 21min. This is
not acceptable, running the whole suite should be in the range of 10-20sec,
otherwise people test even less than they do now.

The bug tracker has a proposed solution with an increasing timeout, this should
really be implemented and it should be checked if the typical case is below
100ms. If this doesn't work out, the test should be done in a fundamentally
different way and if that's not possible we should drop the tests completely
from our suite, the annoyance-to-usefulness ration is a bit too high...

[yurys, 2013-06-13 08:11:56]

On 2013/06/13 06:27:08, Sven Panne wrote:
> I actually don't like this change, for several reasons:
> 
> Bumping up some arbitrary limits is not a solution at all, the flakiness
already
> came back (see our bots). Note that those tests regularly fail on our bots for
> months now, with more than half a dozen half-hearted repair attempts. This is
> really annoying for the whole team, because you constantly get "you broke the
> build" mails for no good reason.
> 
> The other reason why I don't like the change is that the tests are not "good
> team players" in the unit test sense: Any unit test taking longer than 100ms
> (probably even less) is very bad if you consider the runtime of the whole
suite.
> If every test took 500ms and we would be able to perfectly distribute this on
32
> cores, the suite would take 2min35sec, on a 4 core Laptop almost 21min. This
is
> not acceptable, running the whole suite should be in the range of 10-20sec,
> otherwise people test even less than they do now.
>
> The bug tracker has a proposed solution with an increasing timeout, this
should
> really be implemented and it should be checked if the typical case is below
> 100ms. If this doesn't work out, the test should be done in a fundamentally
> different way and if that's not possible we should drop the tests completely
> from our suite, the annoyance-to-usefulness ration is a bit too high...

We share your frustration here. Whenever possible we should use some
deterministic approach in unit tests not depending on any timeouts. We have a
solution on how to remove timeout for this particular case and Ilya is working
on it. Also I'm going to update more CPU profiler tests to force taking samples
from withing the code being profiled instead of relying on the profiling
interval being long enough to collect necessary samples of the code.

We still need at least a few tests for the actual sampler to make sure it is not
broken(which has happened several times already and was discovered a few weeks
later because of poor test coverage of CPU profiler) and I don't see how to make
those tests faster as the profiler has 1ms sampling rate at the moment. Any
ideas on how to address this case are appreciated. In any case I don't think
having no tests is a solution.