+Zhen, I think this is because https://crrev.com/43f84917812c614b2a38e8e27064c6873ca6cdb0 was reverted and some benchmark started using the new API in the meanwhile.

(Full credit to jochen@ who originally spotted this.)

Trying to use startup_url (https://codereview.chromium.org/1797223002) crashes the timeline based measurements:

Traceback (most recent call last):
  RunBenchmark at third_party/catapult/telemetry/telemetry/internal/story_runner.py:302
    Run(pt, stories, finder_options, results, benchmark.max_failures)
  Run at third_party/catapult/telemetry/telemetry/internal/story_runner.py:219
    _RunStoryAndProcessErrorIfNeeded(story, results, state, test)
  _RunStoryAndProcessErrorIfNeeded at third_party/catapult/telemetry/telemetry/internal/story_runner.py:86
    test.Measure(state.platform, results)
  Measure at third_party/catapult/telemetry/telemetry/web_perf/timeline_based_measurement.py:281
    trace_result = platform.tracing_controller.StopTracing()
  StopTracing at third_party/catapult/telemetry/telemetry/core/tracing_controller.py:39
    return self._tracing_controller_backend.StopTracing()
  StopTracing at third_party/catapult/telemetry/telemetry/internal/platform/tracing_controller_backend.py:124
    '\n'.join(raised_exception_messages))
TracingControllerStoppedError: Exceptions raised when trying to stop tracing:
Traceback (most recent call last):
  File "/usr/local/google/home/eisinger/blink/src/third_party/catapult/telemetry/telemetry/internal/platform/tracing_controller_backend.py", line 113, in StopTracing
    agent.StopAgentTracing(builder)
  File "/usr/local/google/home/eisinger/blink/src/third_party/catapult/telemetry/telemetry/internal/platform/tracing_agent/chrome_tracing_agent.py", line 143, in StopAgentTracing
    '\n'.join(raised_execption_messages))
ChromeTracingStoppedError: Exceptions raised when trying to stop Chrome devtool tracing:
Error when trying to stop Chrome tracing on devtools at port 46872:
Traceback (most recent call last):
  File "/usr/local/google/home/eisinger/blink/src/third_party/catapult/telemetry/telemetry/internal/platform/tracing_agent/chrome_tracing_agent.py", line 128, in StopAgentTracing
    client.StopChromeTracing(trace_data_builder)
  File "/usr/local/google/home/eisinger/blink/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py", line 337, in StopChromeTracing
    assert self.is_tracing_running
AssertionError

@zhenw: Can we add the ShouldTearDownStateAfterEachStoryRun back to story_runner? I recall the original idea is that if the benchmark define this, story_runner will always tear down the shared_state & reconstruct the new one in between story runs.

#3: how does that bug related to this?

The commit that introduced ShouldTearDownStateAfterEachStoryRun said that it would replace startup_url. However, ShouldTearDownStateAfterEachStoryRun was reverted, and startup_url regressed.

benchmark.ShouldTearDownStateAfterEachStoryRun was reverted because this API will tear down all states. For startup tracing benchmark, they wanted to keep some states, e.g., the profile states, and then restart the browser. This will then break.

startup_url is currently not supported in TBM benchmarks.


ulan, what are the use cases for v8 and memory infra benchmarks? Do you want to restart the browser after each story run? If you do, it is probably better to create a new bug for that purpose and we can try to find a solution.

And we should remove ShouldTearDownStateAfterEachStoryRun() from those places for now to makes sure people are not confused.

I want a fresh renderer for every page in the story, otherwise all but the first page get to run with common code already compiled and in the cache, but on the downside also more GC pressure.

My benchmark (v8.todomvc) specifically wants to check the startup performance for each page in a clean renderer.

Do you need to reuse some state between story runs? For example, profile state or some other things.

If not, we probably can bring ShouldTearDownStateAfterEachStoryRun() back and specify that this API will tear down all states (more than browser restart). Ned, WDYT?

Actually, there is a way to make startup_url work for TBM.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapult/telemetry/telemetry/internal/platform/tracing_agent/chrome_tracing_agent.py&l=124
s/GetDevToolsClients/GetActiveDevToolsClients

This may not be the correct fix. As indicated by the comments:
# We get all DevTools clients including the stale ones, so that we get an
# exception if there is a stale client. This is because we will potentially
# lose data if there is a stale client.

When using startup_url, the old DevTools client is gone when the browser is restarted. A new DevTools client is created for the new browser. If we want to make startup_url work for TBM, I think the correct fix should be adding chrome_tracing_devtools_manager.UnregisterDevToolsClient(). And call it in devtools_client_backend.Close(). In this way, the DevTools client unregister itself when itself is closed (when the browser is closed). All other stale client is still kept and we get the exception.

Ned, WDYT?

For my use case, discarding all state is fine

Zhen: sgtm. I recall that was what we did last time but got reverted because we also try to apply the new way of restarting browsers on existing startup benchmarks?

the original CL was reverted because blowing away all the state broke warm start tests

Discussed with Ned offline, we will bring back ShouldTearDownStateAfterEachStoryRun API and restrict the use of startup_url. Will put up a CL.

benchmark.ShouldTearDownStateAfterEachStoryRun is now added at https://codereview.chromium.org/1806023002/