So basically we are repeating the same test on both shards for some reason and it isn't clear why. 

If you see the test results for shard 1:
https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=391e98eb25606250e708475e12e29a38ce965a28&as=test_results.json

Shard 2: 
https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=88e33ac2b71a17e34ee36b936c30194398c52298&as=test_results.json

They both run word-wrap-break-word.html

There must be a bug in telemetry somewhere or there is something funny with blink_perf.layout tests.  I am still investigating.  We haven't seen this on any other bots so far that we know of.

What is more interesting is that two different configurations running the same shard map are running different tests for index 0:16.  If you look at shard 1 of pixel 2: https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=fb9bde16eb2131488757ca572c935f158f9e10c0&as=test_results.json

vs android go: 

https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=391e98eb25606250e708475e12e29a38ce965a28&as=test_results.json

They are completely different subsets. 

Now this could be copmletely unrelated to the fact that two shards are running the same test which is the error in this bug.  But we assume for sharding purposes that it is a deterministic set and this is proving that the indices are different based on configuration.

So it turns out that we cannot guarantee story ordering like our begin and end indices implemented in telemetry assume we do.  blink_perf greps a set of directories for story names and this can vary based on configuration and in our case even bot.

Therefore we need to implement something that guarantees alphabetical order in story sets so that they are always run in the same order.  We are proposing doing this at the StorySet level so it is consistent across any users of telemetry.

+Juan: see #3

We do realize for sharding this means that when new stories are added it will break device affinity for at least two stories since it will shift the index by one.  

#6: I believe it's "at most two stories" 

Is this something specific to blink_perf? Or Telemetry in general?

I think in Telemetry (as implemented) the StorySet's are actually Story "lists"? https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/story/story_set.py?rcl=ba9d017c9e77b3e6bcd2bfa7dceeedc9f1de2414&l=34

blink_perf is the only place we have noticed so far.  Even though it is a list, we can't guarantee that the user is going to add them in a deterministic order.

Hmm.. yes, I see what the problem is.

I think the root cause is that Telemetry allows to create these page sets completely dynamically, based on essentially any arbitrary property of the environment in which where they are running (like which files are found on some folder on the host).

But the sharding mechanism wants to see the page sets as "static" things, which can be deterministically generated (maybe even on a platform different than the intended target platform where the tests are meant to run) and stably indexed as a list.

On the short term the blink_perf issue can probably be addressed by changing the page set generation to make sure pages *are* added in a specific pre-determined order.

But the more general problem won't go away even if we say, patch the StorySet object to always iterate over pages alphabetically (*), because the user may still decide to add more or less pages depending on random environment conditions making indices point to different pages on different setups.

What I would like to see in the longer term is moving in the direction where page set can be statically generated as I suggested above.

(*) Note, if we are not careful, this will also break the (few) benchmarks where stories are not independent and the order in which they run is important.

Ok it sounds like there might be a couple of different things wrong with our alphabetical assumption:

1) there are some benchmarks that order they are run is important, so making the change to alphabetize them might actually break the benchmark

2) Users could still choose to add different stories based on different variables so alphabetizing them won't guarantee indices at that point.

I think these invalid assumptions mean we need to have a larger conversation around the --experimental-story-shard-*-index and whether it is still valid or not.

I *think* for sharding purposes if we fix the blink_perf case and re-generate the maps based on that order we might be ok.  I have not verified that this doesn't happen for other benchmarks, this is just the first one we noticed. 

To the point of (2), a benchmark to run on the perf bots is always guaranteed to have same set of variables.

To the point of (1), which are those benchmarks? I recall we kill almost all the benchmarks that have dependency between the stories. Bisect per-story cannot work well if that is the case.

Having said that, I am fine with fix blink_perf to unblock the sharding work, since it seems there are lots of nuances in addressing this issue.

Yes I agree. My thoughts are that this should not affect too many benchmarks, so a one-off fix for blink_perf should be fine for now. Most other benchmarks really just add a hard-coded list of stories to the set.

I'm happy to take this bug after the blink_perf fix to start the larger conversation on what the more general solution should look like.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb4300c3b8e0b2214168c8a9fd9e34b7b5448089

commit eb4300c3b8e0b2214168c8a9fd9e34b7b5448089
Author: Emily Hanley <eyaich@google.com>
Date: Wed Jun 06 20:07:23 2018

Sorting blink_perf story sets to be in a determinisitic order

Bug: 849541
Change-Id: Idbd3c3a0339c933c70a5a2335a1faca5f507eb72
Reviewed-on: https://chromium-review.googlesource.com/1089118
Commit-Queue: Emily Hanley <eyaich@chromium.org>
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Cr-Commit-Position: refs/heads/master@{#565014}
[modify] https://crrev.com/eb4300c3b8e0b2214168c8a9fd9e34b7b5448089/tools/perf/benchmarks/blink_perf.py

Ah, I didn't see comment in #12 before my reply in #13.

On the point (2) because of the current StorySet.AddPage API, users are free to dynamically generate story sets based on *whatever* conditions they want (not just bot configs). An extreme example:

  if today is Monday:
    self.AddPage('foo')
  else:
    self.AddPage('bar')

This is of course silly, but points out (a) the sort of pitfalls we need to avoid [the blink_perf example where the pages that get added depend on files that may or may not exist on the host is not that different] and (b) the issue is not only about the *order* of the pages in the set; but also about which pages get added to it.

My (half-baked) thoughts at the moment on how to solve this is that some properties of stories (like their names) should be accessible on the *class* even before instantiating the object (this is already true e.g. of system health stories); so the story-set can then literally be a hard-coded list of story-classes.

Of course you could still do crazy dynamic things (in the end classes are also just objects), but it would be far less easy to make mistakes in the most common case (because then it's hard to accidentally depend on values from the instantiated objects), and if some dynamism is needed it will be easier to make more explicit which particular bot config details affect the generated story sets.

On the point (1) both memory.top_10_mobile (requires foreground/background pairs of stories to run in that order) and memory.dual_browser_test (requires chrome/webview pairs of stories to run in that order). - Note I think we should *also* fix this, and remove oddities in these benchmarks; but that won't solve the problems of (2) above.