Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1872313002:
sandwich: Implement SandwichTaskBuilder (Closed)
|
Created:
4 years, 8 months ago by gabadie Modified:
4 years, 7 months ago Reviewers:
pasko CC:
chromium-reviews, mikecase+watch_chromium.org, gabadie+watch_chromium.org, jbudorick+watch_chromium.org, lizeb+watch-android-loading_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionsandwich: Implement SandwichTaskBuilder
Sandwich automation is moving to the task_manager API. This CL
implements the SandwichTaskBuilder that builds the sandwich's
task dependency graph.
BUG=582080
Committed: https://crrev.com/e88c3b13065fac6f02ec54b95c3fe66ee89f0c37
Cr-Commit-Position: refs/heads/master@{#390048}
Patch Set 1 #
Total comments: 52
Patch Set 2 : Addresses pasko's comments and adds support for the different sub-resources discovrers #
Total comments: 32
Patch Set 3 : Rebase #Patch Set 4 : Addresses pasko's comments #Patch Set 5 : mv sandwich_tasks.py -> sandwich_task_builder.py #
Total comments: 45
Patch Set 6 : Rebase #Patch Set 7 : Addresses pasko's comments #
Total comments: 19
Patch Set 8 : Rebase #Patch Set 9 : Addresses pasko's comments #
Total comments: 28
Patch Set 10 : Rebase #Patch Set 11 : Addresses pasko's comment #
Total comments: 5
Patch Set 12 : Rebase #Patch Set 13 : Addresses pasko's comment #Patch Set 14 : Raises RuntimeError when encountering an unknown protocol #
Messages
Total messages: 23 (4 generated)
gabadie@chromium.org changed reviewers: + pasko@chromium.org
Hey Egor, Now the CL that use the task manager. PTAL =D
looks mostly what I would expect, so here is the first round of low-level-ish comments Generation of one-parameter functions still looks like a weird part of the SandwichTaskBuilder interface. We can leave it as is right now, but later I would be tempted to move it inside the builder. Some temporary state for the builder is not worse than this wrapped state in the curried function. Let's see how it goes with adding experimentation modes. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:176: help='Run the sandwich orchestra.') 'Run all steps using the task manager infrastructure. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:178: help='Generate the full orchestra.') there is no more orchestra. For now we can say: 'Generate the full graph with all possible network simulation types.' https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:181: help='Web page replay archive to load job\'s urls ' help='WebPageReplay archive to use, instead of generating one' https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:266: '2g': sandwich_tasks.EmulateNetworkModifier('Regular2G'), Let's rename it to something like NetworkSimulationTransformer we can actually avoid this dict of runner_modifiers and deal with '2g' -> 'Regular2G' transition in the callback: ... for type in sandwich_tasks.SUPPORTED_NETWORK_TYPES: transformer = sandwich_tasks.CreateNetworkSimulationTrasformer(type) builder.PopulateClearCacheLoadBenchmark( benchmark_name='clearcache-' + type, sandwich_runner_transformer=transformer) builder.PopulateNoStatePrefetchLoadBenchmark( ... ...) I think 'transformer' more explicitly suggests that it would take the sandwich runner as parameter and would change its state, while a 'modifier' could be interpreted as a 'small addition' .. like those regexp modifiers. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_tasks.py (right): https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:22: def NoRunnerModification(runner): This would be a NoOpTransformer, but actually I would prefer to avoid having a small function like this, we can just: builder.PopulateNoStatePrefetchLoadBenchmark( sandwich_runner_transformer=lambda arg: None) https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:31: def EmulateNetworkModifier(network_condition): As mentioned before, NetworkSimulationTransformer would be more appropriate as a name https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:32: """Factory that create callbacks to modify a sandwich runner for a specific Creates a function that accepts a SandwichRunner as a parameter and sets network emulation options on it. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:39: A callback modifying the runner given in argument accordingly needs to mention |SandwichRunner| https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:42: def RunnerModifier(runner): s/runner/sandwich_runner/ https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:43: runner.network_condition = network_condition assert isinstance(sandwich_runner, SandwichRunner) https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:48: """Sandwich's tasks builder.""" Needs more documentation. Something like: """A builder for a graph of tasks, each prepares or invokes the SandwichRunner. Also, we need a list of files/directories the task builder manages (limited to final targets). It can be at the top of the file, or here. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:54: output_directory: Output directory where the dynamic tasks will be output_directory: As in task_manager.Builder.__init__ job_path: ... url_repeat: ... https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:60: self.default_final_tasks = [] we want to avoid callers of this class from modifying default_final_tasks, so it would be better to have: def GetFinalTasks(self): return self._default_final_tasks https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:62: def __enter__(self): not needed? https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:65: def __exit__(self, exc_type, exc_val, exc_tb): not needed any more? https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:68: def _CreateRunner(self): This methd is called only once, it is preferable to inline it because the helpful work it does is not obvious from the name. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:75: def _CreateNonBenchmarkRunner(self): _CreateSandwichRunner to be clearer https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:81: def SetOriginalWprPath(self, original_wpr_path): It is not clear what 'original' refers to. It would be clearer to: OverridePathToWprArchive() https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:87: return self.CreateStaticTask('webpages.wpr', original_wpr_path) 'webpages.wpr' -> _WPR_ARCHIVE_NAME as a constant to avoid typos. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:89: def PopulateWPRRecordingTask(self): naming: s/WPR/Wpr/ https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:101: """Populates the pipeline that create the reference cache archive and list It makes sense to explain in each Populate* what subgraph it creates. It would help readability because this code is 'optimized' for understanding what individual tasks do, but to make sense of all of them together, one needs to look at the .dot, and there you cannot see what each endividual Populate* function did, so I'd suggest to provide some help for the reader here, like: cache-ref-validation.log depends on: cache-ref.zip depends on: webpages-patched.wpr depends on: webpages.wpr depends on: urls-resources.json depends on: urls-resources-run/ depends on: webpages.wpr https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:124: # task that can generate several files at a time. It is not clear what this TODO proposes, maybe remove it? As far as I can tell it does not need 'octopus dynamic task', it would be enough to allow to have a dict of assets that a task produces, and then two tasks can grab two outputs by name. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:133: @self.RegisterTask('urls-resources.json', [UrlsResourcesRun]) subresources-for-urls.json would wake it clear where resources come from, right now it sounds like some urls and some resources with unknown relation to each other https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:214: Args: need args here https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:275: urls_resources_run_task = self.tasks['urls-resources-run/'] urls-resources-run/ and urls-resources.json are used several times in this file, consider using constants
Oups, forgot to submit. I have addressed your comments in the new revision. PTAL. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:176: help='Run the sandwich orchestra.') On 2016/04/11 14:54:08, pasko wrote: > 'Run all steps using the task manager infrastructure. Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:178: help='Generate the full orchestra.') On 2016/04/11 14:54:08, pasko wrote: > there is no more orchestra. For now we can say: > 'Generate the full graph with all possible network simulation types.' Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:181: help='Web page replay archive to load job\'s urls ' On 2016/04/11 14:54:08, pasko wrote: > help='WebPageReplay archive to use, instead of generating one' Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich.py:266: '2g': sandwich_tasks.EmulateNetworkModifier('Regular2G'), On 2016/04/11 14:54:08, pasko wrote: > Let's rename it to something like NetworkSimulationTransformer > > we can actually avoid this dict of runner_modifiers and deal with '2g' -> > 'Regular2G' transition in the callback: > > ... > > for type in sandwich_tasks.SUPPORTED_NETWORK_TYPES: > transformer = sandwich_tasks.CreateNetworkSimulationTrasformer(type) > builder.PopulateClearCacheLoadBenchmark( > benchmark_name='clearcache-' + type, > sandwich_runner_transformer=transformer) > builder.PopulateNoStatePrefetchLoadBenchmark( > ... > ...) > > I think 'transformer' more explicitly suggests that it would take the sandwich > runner as parameter and would change its state, while a 'modifier' could be > interpreted as a 'small addition' .. like those regexp modifiers. Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_tasks.py (right): https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:22: def NoRunnerModification(runner): On 2016/04/11 14:54:08, pasko wrote: > This would be a NoOpTransformer, but actually I would prefer to avoid having a > small function like this, we can just: > > builder.PopulateNoStatePrefetchLoadBenchmark( > sandwich_runner_transformer=lambda arg: None) Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:31: def EmulateNetworkModifier(network_condition): On 2016/04/11 14:54:08, pasko wrote: > As mentioned before, NetworkSimulationTransformer would be more appropriate as a > name Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:32: """Factory that create callbacks to modify a sandwich runner for a specific On 2016/04/11 14:54:08, pasko wrote: > Creates a function that accepts a SandwichRunner as a parameter and sets network > emulation options on it. Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:39: A callback modifying the runner given in argument accordingly On 2016/04/11 14:54:08, pasko wrote: > needs to mention |SandwichRunner| Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:42: def RunnerModifier(runner): On 2016/04/11 14:54:09, pasko wrote: > s/runner/sandwich_runner/ Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:43: runner.network_condition = network_condition On 2016/04/11 14:54:08, pasko wrote: > assert isinstance(sandwich_runner, SandwichRunner) Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:48: """Sandwich's tasks builder.""" On 2016/04/11 14:54:09, pasko wrote: > Needs more documentation. > > Something like: > """A builder for a graph of tasks, each prepares or invokes the SandwichRunner. Done. > > Also, we need a list of files/directories the task builder manages (limited to > final targets). It can be at the top of the file, or here. I don't think so, because it is pretty straight forward with the dependency graph visualization, but the comment may become out-dated. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:54: output_directory: Output directory where the dynamic tasks will be On 2016/04/11 14:54:09, pasko wrote: > output_directory: As in task_manager.Builder.__init__ > job_path: ... > url_repeat: ... Oups... Done! https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:60: self.default_final_tasks = [] On 2016/04/11 14:54:08, pasko wrote: > we want to avoid callers of this class from modifying default_final_tasks, so it > would be better to have: > def GetFinalTasks(self): > return self._default_final_tasks used a @proprety. Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:62: def __enter__(self): On 2016/04/11 14:54:08, pasko wrote: > not needed? My bad. Done https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:65: def __exit__(self, exc_type, exc_val, exc_tb): On 2016/04/11 14:54:08, pasko wrote: > not needed any more? Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:68: def _CreateRunner(self): On 2016/04/11 14:54:08, pasko wrote: > This methd is called only once, it is preferable to inline it because the > helpful work it does is not obvious from the name. Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:75: def _CreateNonBenchmarkRunner(self): On 2016/04/11 14:54:09, pasko wrote: > _CreateSandwichRunner to be clearer Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:81: def SetOriginalWprPath(self, original_wpr_path): On 2016/04/11 14:54:08, pasko wrote: > It is not clear what 'original' refers to. It would be clearer to: > OverridePathToWprArchive() Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:87: return self.CreateStaticTask('webpages.wpr', original_wpr_path) On 2016/04/11 14:54:09, pasko wrote: > 'webpages.wpr' -> _WPR_ARCHIVE_NAME as a constant to avoid typos. Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:89: def PopulateWPRRecordingTask(self): On 2016/04/11 14:54:09, pasko wrote: > naming: s/WPR/Wpr/ Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:101: """Populates the pipeline that create the reference cache archive and list On 2016/04/11 14:54:08, pasko wrote: > It makes sense to explain in each Populate* what subgraph it creates. > > It would help readability because this code is 'optimized' for understanding > what individual tasks do, but to make sense of all of them together, one needs > to look at the .dot, and there you cannot see what each endividual Populate* > function did, so I'd suggest to provide some help for the reader here, like: > > cache-ref-validation.log > depends on: cache-ref.zip > depends on: webpages-patched.wpr > depends on: webpages.wpr > depends on: urls-resources.json > depends on: urls-resources-run/ > depends on: webpages.wpr Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:124: # task that can generate several files at a time. On 2016/04/11 14:54:09, pasko wrote: > It is not clear what this TODO proposes, maybe remove it? > > As far as I can tell it does not need 'octopus dynamic task', it would be enough > to allow to have a dict of assets that a task produces, and then two tasks can > grab two outputs by name. Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:133: @self.RegisterTask('urls-resources.json', [UrlsResourcesRun]) On 2016/04/11 14:54:08, pasko wrote: > subresources-for-urls.json would wake it clear where resources come from, right > now it sounds like some urls and some resources with unknown relation to each > other Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:214: Args: On 2016/04/11 14:54:08, pasko wrote: > need args here My bad. Done. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:275: urls_resources_run_task = self.tasks['urls-resources-run/'] On 2016/04/11 14:54:08, pasko wrote: > urls-resources-run/ and urls-resources.json are used several times in this file, > consider using constants Done.
overall looks right, following up with more small comments. Sharing comments early before I read through all of the change. Will continue reading in parallel. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_tasks.py (right): https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:60: self.default_final_tasks = [] On 2016/04/13 09:53:44, gabadie wrote: > On 2016/04/11 14:54:08, pasko wrote: > > we want to avoid callers of this class from modifying default_final_tasks, so > it > > would be better to have: > > def GetFinalTasks(self): > > return self._default_final_tasks > > used a @proprety. Done. did you want to allow users of the object to overwrite the list? I think that is not desired, in which case it would be preferable to disallow modifications on the class API level. @property would be too verbose for it, that is why I suggested a simple getter. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/c... File tools/android/loading/common_util.py (right): https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/c... tools/android/loading/common_util.py:94: def VerifyOrCreateParentDirectory(path): nit: usually when I saw things like that they were named as: EnsureParentDirectoryExists(path) up to you https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/c... tools/android/loading/common_util.py:95: """Verifies that the parent directory exists or create it if missing.""" s/create/creates/ https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich.py:280: for subresources_discoverer in sandwich_misc.SUBRESOURCE_DISCOVERERS: s/subresources_discoverer/subresource_discoverer/ https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:17: # Don't prefetch anything # Do not prefetch anything. (i.e. a full stop at the end) https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:18: DISABLED_DISCOVERER = 'disabled' is this used as part of file names? if so, seeing 'disabled' as a file name would not obviously suggest _what_ is disabled. So seems this would be better: 'no-discoverer' or 'nocache'. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:21: FULLCACHE_DISCOVERER = 'fullcache' nit: prefer words separated by dashes in file names: 'no-cache', 'full-cache'. This is minor, up to you. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... File tools/android/loading/sandwich_tasks.py (right): https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. let's name the file sandwich_task_builder to make it more obvious where the SandwichTaskBuilder is defined? https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:15: import loading_trace unneeded import? please clean up other unneeded imports https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:34: def RunnerModifier(sandwich_runner): this would be slightly more elegant: def Transformer(sandwich_runner): .. return Transformer https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:96: """Populates the pipeline that create the reference cache archive and list This should preferably be a one-liner, so: """Creates necessary tasks to produce initial cache archive. Also creates a task for producing a json file with a mapping of URLs to subresources (urls-resources.json). Here is the full dependency tree for the returned task: common/cache-ref-validation.log depends on: ... https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:100: common/depends on: cache-ref.zip depends on: common/cache-ref.zip https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:164: runner_transformer: A closure to transform the sandwich runner. runner_transformer_name: ... https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:164: runner_transformer: A closure to transform the sandwich runner. it's actually not a closure :( runner_transformer: A function that would be applied once to SandwichRunner before it is run. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:168: The last task of the pipeline. s/task/Task/ https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:170: assert subresources_discoverer in sandwich_misc.SUBRESOURCE_DISCOVERERS s/subresources/subresource/ https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:172: shared_task_preffix = os.path.join('shared', subresources_discoverer) s/preffix/prefix/
Thanks Egor. Addressed all your comments in the new CLs. PTAL. https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_tasks.py (right): https://codereview.chromium.org/1872313002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_tasks.py:60: self.default_final_tasks = [] On 2016/04/14 12:34:42, pasko wrote: > On 2016/04/13 09:53:44, gabadie wrote: > > On 2016/04/11 14:54:08, pasko wrote: > > > we want to avoid callers of this class from modifying default_final_tasks, > so > > it > > > would be better to have: > > > def GetFinalTasks(self): > > > return self._default_final_tasks > > > > used a @proprety. Done. > > did you want to allow users of the object to overwrite the list? I think that is > not desired, in which case it would be preferable to disallow modifications on > the class API level. @property would be too verbose for it, that is why I > suggested a simple getter. Sorry, I still don't see the issue with a simple getter with a @property. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/c... File tools/android/loading/common_util.py (right): https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/c... tools/android/loading/common_util.py:94: def VerifyOrCreateParentDirectory(path): On 2016/04/14 12:34:42, pasko wrote: > nit: usually when I saw things like that they were named as: > EnsureParentDirectoryExists(path) > > up to you Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/c... tools/android/loading/common_util.py:95: """Verifies that the parent directory exists or create it if missing.""" On 2016/04/14 12:34:42, pasko wrote: > s/create/creates/ Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich.py:280: for subresources_discoverer in sandwich_misc.SUBRESOURCE_DISCOVERERS: On 2016/04/14 12:34:42, pasko wrote: > s/subresources_discoverer/subresource_discoverer/ Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:17: # Don't prefetch anything On 2016/04/14 12:34:42, pasko wrote: > # Do not prefetch anything. > > (i.e. a full stop at the end) Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:18: DISABLED_DISCOVERER = 'disabled' On 2016/04/14 12:34:42, pasko wrote: > is this used as part of file names? if so, seeing 'disabled' as a file name > would not obviously suggest _what_ is disabled. > > So seems this would be better: 'no-discoverer' or 'nocache'. Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:21: FULLCACHE_DISCOVERER = 'fullcache' On 2016/04/14 12:34:42, pasko wrote: > nit: prefer words separated by dashes in file names: 'no-cache', 'full-cache'. > > This is minor, up to you. Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... File tools/android/loading/sandwich_tasks.py (right): https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/04/14 12:34:42, pasko wrote: > let's name the file sandwich_task_builder to make it more obvious where the > SandwichTaskBuilder is defined? Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:15: import loading_trace On 2016/04/14 12:34:43, pasko wrote: > unneeded import? > > please clean up other unneeded imports Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:34: def RunnerModifier(sandwich_runner): On 2016/04/14 12:34:43, pasko wrote: > this would be slightly more elegant: > > def Transformer(sandwich_runner): > .. > return Transformer Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:96: """Populates the pipeline that create the reference cache archive and list On 2016/04/14 12:34:43, pasko wrote: > This should preferably be a one-liner, so: > """Creates necessary tasks to produce initial cache archive. > > Also creates a task for producing a json file with a mapping of URLs to > subresources (urls-resources.json). > > Here is the full dependency tree for the returned task: > common/cache-ref-validation.log > depends on: ... Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:100: common/depends on: cache-ref.zip On 2016/04/14 12:34:43, pasko wrote: > depends on: common/cache-ref.zip Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:164: runner_transformer: A closure to transform the sandwich runner. On 2016/04/14 12:34:43, pasko wrote: > runner_transformer_name: ... Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:164: runner_transformer: A closure to transform the sandwich runner. On 2016/04/14 12:34:42, pasko wrote: > it's actually not a closure :( > > runner_transformer: A function that would be applied once to SandwichRunner > before it is run. functions are subsets of closures, and it will be very likely be used as a closure. Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:168: The last task of the pipeline. On 2016/04/14 12:34:43, pasko wrote: > s/task/Task/ Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:170: assert subresources_discoverer in sandwich_misc.SUBRESOURCE_DISCOVERERS On 2016/04/14 12:34:43, pasko wrote: > s/subresources/subresource/ Done. https://codereview.chromium.org/1872313002/diff/20001/tools/android/loading/s... tools/android/loading/sandwich_tasks.py:172: shared_task_preffix = os.path.join('shared', subresources_discoverer) On 2016/04/14 12:34:43, pasko wrote: > s/preffix/prefix/ Done.
some more comments (from last Friday) https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich.py:54: orchestra_parser = task_manager.CommandLineParser() last instance of orchestra :) https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich.py:276: builder.PopulateLoadBenchmark(sandwich_misc.NO_DISCOVERER) this would create: 1. output/dummy/no-discoverer-run/ and 2. output/dummy/no-discoverer-metrics.json right? The other would: 1. output/dummy/full-cache-run/ and 2. output/dummy/full-cache-metrics.json A few issues: (A) it would be unclear what 'dummy' means. It should be obvious from the name that we mean network emulation. Suggesting: 'fastest-network', 'no-network-emulation'. (B) 'no-discoverer' does not look nice in this context - 'empty-cache' would be better (my previous suggestion was not taking this directory structure into account - sry) (C) too many default arguments for the job, explicitly passing params makes it easier to reason about this use of the function, and only adds 4 lines of params (and 2 lines for defining a string constant - oh my) https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich.py:287: builder.PopulateLoadBenchmark(subresource_discoverer, nit: this call looks 33% shorter with all params on the next line https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:133: def CompareUrlSet(ref_url_set, url_set, url_set_name, debug_hint='Good luck!'): I find this debug hint to be slightly offensive :) please change it to '' https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:133: def CompareUrlSet(ref_url_set, url_set, url_set_name, debug_hint='Good luck!'): why is this function in sandwich_misc? is it going to be used in the integration test? if not, then it's just an overhead for switching between files when reading the code. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:133: def CompareUrlSet(ref_url_set, url_set, url_set_name, debug_hint='Good luck!'): not used outside sandwich_misc -> _Compare... https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:133: def CompareUrlSet(ref_url_set, url_set, url_set_name, debug_hint='Good luck!'): more intuitive name: PrintUrlSetComparison A function named as CompareX suggests that it returns a result of comparison and does not have side effects. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:135: need to explain what the function prints https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:146: logging.info(' %d %s are matching.' % (len(ref_url_set), url_set_name)) why % formatting here and {} formatting in other places? Is it just by accident or there is some clever reason for not being consistent? https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:158: def _ListUrlRequests(trace, from_cache=None): default arguments that can be True, False and None are confusing, especially without a pydoc. What does this argument mean? https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:176: benchmark_setup_path: Path of the JSON of the benchmark setup. what is a 'benchmark setup'? https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:191: trace_path = os.path.join(run_path, 'trace.json') 'trace.json' exists in many files, consider making a constant for it in common_util https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:197: CompareUrlSet(url_resources, _ListUrlRequests(trace), 'All resources', explicit second arg for _ListUrlRequests is needed, implicit stuff is more error prone https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:198: 'You may have an issue with an AJAX requests.') I think one can easily tell this by looking at URLs, is this hint useful? https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:201: 'The WPR archive patcher may have an invalidation issue.') It's slightly funny to see gabadie@ providing hints for himself, well .. hmm, was this hint useful to you? https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:206: def ListResourcesUrls(benchmark_output_directory_path): ListResourceUrls https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:207: """Lists all requested urls per navigated urls s/urls/URLs/ https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... File tools/android/loading/sandwich_misc_unittest.py (right): https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc_unittest.py:22: def testNoDiscovrerWhitelisting(self): s/Discovrer/Discoverer/ https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc_unittest.py:23: urls_set_ref = set() variable not needed, just inline set() in assertEquals https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc_unittest.py:29: urls_set_ref = set([self.GetResourceUrl('./'), reference_url_set https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc_unittest.py:33: urls_set = sandwich_misc.ExtractDiscoverableUrls( url_set https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... File tools/android/loading/sandwich_task_builder.py (right): https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_task_builder.py:175: task_prefix = os.path.join(runner_transformer_name, why os.path.join? I thought all tasks are immediate subdirectories of output
Thanks Egor! New revision uploaded. PTAL! =D https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... File tools/android/loading/sandwich.py (right): https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich.py:54: orchestra_parser = task_manager.CommandLineParser() On 2016/04/18 09:36:12, pasko wrote: > last instance of orchestra :) Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich.py:276: builder.PopulateLoadBenchmark(sandwich_misc.NO_DISCOVERER) On 2016/04/18 09:36:12, pasko wrote: > this would create: > 1. output/dummy/no-discoverer-run/ and > 2. output/dummy/no-discoverer-metrics.json > right? > > The other would: > 1. output/dummy/full-cache-run/ and > 2. output/dummy/full-cache-metrics.json > > A few issues: > > (A) it would be unclear what 'dummy' means. It should be obvious from the name > that we mean network emulation. Suggesting: 'fastest-network', > 'no-network-emulation'. > > (B) 'no-discoverer' does not look nice in this context - 'empty-cache' would be > better (my previous suggestion was not taking this directory structure into > account - sry) > > (C) too many default arguments for the job, explicitly passing params makes it > easier to reason about this use of the function, and only adds 4 lines of params > (and 2 lines for defining a string constant - oh my) > Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich.py:287: builder.PopulateLoadBenchmark(subresource_discoverer, On 2016/04/18 09:36:12, pasko wrote: > nit: this call looks 33% shorter with all params on the next line Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:133: def CompareUrlSet(ref_url_set, url_set, url_set_name, debug_hint='Good luck!'): On 2016/04/18 09:36:12, pasko wrote: > I find this debug hint to be slightly offensive :) > please change it to '' Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:133: def CompareUrlSet(ref_url_set, url_set, url_set_name, debug_hint='Good luck!'): On 2016/04/18 09:36:13, pasko wrote: > more intuitive name: PrintUrlSetComparison > > A function named as CompareX suggests that it returns a result of comparison and > does not have side effects. Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:133: def CompareUrlSet(ref_url_set, url_set, url_set_name, debug_hint='Good luck!'): On 2016/04/18 09:36:13, pasko wrote: > why is this function in sandwich_misc? is it going to be used in the integration > test? if not, then it's just an overhead for switching between files when > reading the code. Not sure to understand. Used only in this file. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:133: def CompareUrlSet(ref_url_set, url_set, url_set_name, debug_hint='Good luck!'): On 2016/04/18 09:36:13, pasko wrote: > not used outside sandwich_misc -> _Compare... Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:135: On 2016/04/18 09:36:12, pasko wrote: > need to explain what the function prints Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:146: logging.info(' %d %s are matching.' % (len(ref_url_set), url_set_name)) On 2016/04/18 09:36:13, pasko wrote: > why % formatting here and {} formatting in other places? Is it just by accident > or there is some clever reason for not being consistent? There is an annoying check in pylint to force the use of % in logging.info() :( https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:158: def _ListUrlRequests(trace, from_cache=None): On 2016/04/18 09:36:13, pasko wrote: > default arguments that can be True, False and None are confusing, especially > without a pydoc. What does this argument mean? Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:176: benchmark_setup_path: Path of the JSON of the benchmark setup. On 2016/04/18 09:36:13, pasko wrote: > what is a 'benchmark setup'? See SetupBenchmark in sandwich_task_builder.py https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:191: trace_path = os.path.join(run_path, 'trace.json') On 2016/04/18 09:36:13, pasko wrote: > 'trace.json' exists in many files, consider making a constant for it in > common_util Nop. trace.json is sandwich specific. I don't see the point in exposing that in common_util. Added in sandwich_runner instead. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:197: CompareUrlSet(url_resources, _ListUrlRequests(trace), 'All resources', On 2016/04/18 09:36:13, pasko wrote: > explicit second arg for _ListUrlRequests is needed, implicit stuff is more error > prone Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:198: 'You may have an issue with an AJAX requests.') On 2016/04/18 09:36:13, pasko wrote: > I think one can easily tell this by looking at URLs, is this hint useful? Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:201: 'The WPR archive patcher may have an invalidation issue.') On 2016/04/18 09:36:13, pasko wrote: > It's slightly funny to see gabadie@ providing hints for himself, well .. hmm, > was this hint useful to you? The point has to give hint to user who wanted to reproduce our analysis. Happy to remove. Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:206: def ListResourcesUrls(benchmark_output_directory_path): On 2016/04/18 09:36:13, pasko wrote: > ListResourceUrls Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc.py:207: """Lists all requested urls per navigated urls On 2016/04/18 09:36:12, pasko wrote: > s/urls/URLs/ Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... File tools/android/loading/sandwich_misc_unittest.py (right): https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc_unittest.py:22: def testNoDiscovrerWhitelisting(self): On 2016/04/18 09:36:13, pasko wrote: > s/Discovrer/Discoverer/ Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc_unittest.py:23: urls_set_ref = set() On 2016/04/18 09:36:13, pasko wrote: > variable not needed, just inline set() in assertEquals Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc_unittest.py:29: urls_set_ref = set([self.GetResourceUrl('./'), On 2016/04/18 09:36:13, pasko wrote: > reference_url_set Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_misc_unittest.py:33: urls_set = sandwich_misc.ExtractDiscoverableUrls( On 2016/04/18 09:36:13, pasko wrote: > url_set Done. https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... File tools/android/loading/sandwich_task_builder.py (right): https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_task_builder.py:175: task_prefix = os.path.join(runner_transformer_name, On 2016/04/18 09:36:13, pasko wrote: > why os.path.join? I thought all tasks are immediate subdirectories of output With the growing number benchmarks, I have added an intermediate directory per runner transformer for easier debuging of the different bench mark setups.
https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... File tools/android/loading/sandwich_task_builder.py (right): https://codereview.chromium.org/1872313002/diff/80001/tools/android/loading/s... tools/android/loading/sandwich_task_builder.py:175: task_prefix = os.path.join(runner_transformer_name, On 2016/04/19 17:39:49, gabadie wrote: > On 2016/04/18 09:36:13, pasko wrote: > > why os.path.join? I thought all tasks are immediate subdirectories of output > > With the growing number benchmarks, I have added an intermediate directory per > runner transformer for easier debuging of the different bench mark setups. Ack. SG, I have not developed my personal taste on it yet, so relying on yours. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... File tools/android/loading/sandwich_task_builder.py (right): https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:162: runner_transformer: A closure that would be applied once to SandwichRunner This is not a closure. "A function that takes an instance of SandwichRunner as parameter, would be applied immediately before SandwichRunner.Run()" https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:163: before it is run. 4 space indent for arg descriptions https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:169: The last task_manager.Task of the pipeline. Plz add the dependency tree for the returned task just as you did with PopulateCommonPipelines(). https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:172: assert 'shared' not in sandwich_misc.SUBRESOURCE_DISCOVERERS we have 'shared' and 'common' as directories, which is confusing to see, we should either unify them into one or draw the distinction in the names. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:175: subresource_discoverer) fits on one line https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:177: @self.RegisterTask(shared_task_prefix + '-setup.json', merge=True, Making sure that merge=True is not misused is complicated. I think this would be simpler: 1. Have a PopulateSomething for SetupBenchmark() and BuildBenchmarkCacheArchive() 2. keep a map of {discoverer: cache_archive_task} in sandwich_task_builder.py, updated by PopulateSomething 3. Populating RunBenchmark takes that task. It moves the logic from generic to this specific class, which would be less elegant, but also less error prone. Let's also discuss this offline. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:187: "This recipe is not ready for multiple urls." no need in this explanation, the assert is enough. You can put a TODO(gabadie): Implement support for multiple URLs in this Task.
New revision uploaded. PTAL. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... File tools/android/loading/sandwich_task_builder.py (right): https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:162: runner_transformer: A closure that would be applied once to SandwichRunner On 2016/04/20 18:57:22, pasko wrote: > This is not a closure. > > "A function that takes an instance of SandwichRunner as parameter, would be > applied immediately before SandwichRunner.Run()" I insist on the closure. A function is a closure but with no mapping associating variable names to ones defined in an enclosing scope (excluding the global scope of course). Here the user may pass down a closure. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:163: before it is run. On 2016/04/20 18:57:22, pasko wrote: > 4 space indent for arg descriptions Done. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:169: The last task_manager.Task of the pipeline. On 2016/04/20 18:57:22, pasko wrote: > Plz add the dependency tree for the returned task just as you did with > PopulateCommonPipelines(). Done. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:172: assert 'shared' not in sandwich_misc.SUBRESOURCE_DISCOVERERS On 2016/04/20 18:57:22, pasko wrote: > we have 'shared' and 'common' as directories, which is confusing to see, we > should either unify them into one or draw the distinction in the names. Merged shared into common. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:175: subresource_discoverer) On 2016/04/20 18:57:22, pasko wrote: > fits on one line My bad. Done. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:177: @self.RegisterTask(shared_task_prefix + '-setup.json', merge=True, On 2016/04/20 18:57:22, pasko wrote: > Making sure that merge=True is not misused is complicated. > > I think this would be simpler: > 1. Have a PopulateSomething for SetupBenchmark() and > BuildBenchmarkCacheArchive() > 2. keep a map of {discoverer: cache_archive_task} in sandwich_task_builder.py, > updated by PopulateSomething > 3. Populating RunBenchmark takes that task. > > It moves the logic from generic to this specific class, which would be less > elegant, but also less error prone. > > Let's also discuss this offline. As agreed offline, let's post pone this in a separate CL. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:187: "This recipe is not ready for multiple urls." On 2016/04/20 18:57:22, pasko wrote: > no need in this explanation, the assert is enough. You can put a TODO(gabadie): > Implement support for multiple URLs in this Task. Done.
https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... File tools/android/loading/sandwich_task_builder.py (right): https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:162: runner_transformer: A closure that would be applied once to SandwichRunner On 2016/04/21 09:32:46, gabadie wrote: > On 2016/04/20 18:57:22, pasko wrote: > > This is not a closure. > > > > "A function that takes an instance of SandwichRunner as parameter, would be > > applied immediately before SandwichRunner.Run()" > > I insist on the closure. A function is a closure but with no mapping associating > variable names to ones defined in an enclosing scope (excluding the global scope > of course). > > Here the user may pass down a closure. As discussed offline, 'closure' can be confusing for people familiar with chromium base/, where it means a callback without free arguments. So, let's refer to it as a function. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:177: @self.RegisterTask(shared_task_prefix + '-setup.json', merge=True, On 2016/04/21 09:32:46, gabadie wrote: > On 2016/04/20 18:57:22, pasko wrote: > > Making sure that merge=True is not misused is complicated. > > > > I think this would be simpler: > > 1. Have a PopulateSomething for SetupBenchmark() and > > BuildBenchmarkCacheArchive() > > 2. keep a map of {discoverer: cache_archive_task} in sandwich_task_builder.py, > > updated by PopulateSomething > > 3. Populating RunBenchmark takes that task. > > > > It moves the logic from generic to this specific class, which would be less > > elegant, but also less error prone. > > > > Let's also discuss this offline. > > As agreed offline, let's post pone this in a separate CL. Acknowledged. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:187: "This recipe is not ready for multiple urls." On 2016/04/21 09:32:46, gabadie wrote: > On 2016/04/20 18:57:22, pasko wrote: > > no need in this explanation, the assert is enough. You can put a > TODO(gabadie): > > Implement support for multiple URLs in this Task. > > Done. Really done? https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:162: None to list all requested urls; these rules are hard to remember, so the reader would have to always look up the function definition when the function is called. More enum-like interface would be good: class _RequestOutcome: All, ServedFromCache, NotServedFromCache = range(3) then: def _ListUrlRequests(trace, request_kind): """... Args: request_kind: _RequestOutcome indicating the subset of requests to output. then to call it: _ListUrlRequests(trace, _RequestOutcome.ServedFromCache) https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:173: if not request_event.protocol.startswith('http'): This was not mentioned in the docstring. Does this filter out data requests? Let's filter out data urls. I really prefer a blacklist. This would allow deciding what to do with other protocols as they appear case-by-case. Having a blind whitelist for http/https would make us silently produce wrong data in such cases. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:181: def VerifyBenchmarkOutputDirectory(benchmark_setup_path, Need to apply the same action as for ValidateCacheArchiveContent() - i.e. TODO or raise exception or put status in the final CSV. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:216: def ListResourceUrls(benchmark_output_directory_path): ReadSubresourceMapFromBenchmarkOutput(...) https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:217: """Lists all requested URLs per navigated URLs """Extracts a map URL-to-subresources for each navigation in benchmark directory.""" https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:251: def ValidateCacheArchiveContent(ref_urls, cache_archive_path): Producing log messages on error is insufficient - easy to miss when running all benchmarks. I see these options: 1. raise an exception and discard the whole Task scenario if cache content is inconsistent. This is simplest to do, and probably good enough because other benchmarks would probably be flawed and the data be useless. 2. Continue but put the result of validation in an intermediate .json and propagate it to the final CSV. More involved. 3. Put a TODO for the next CL. I am OK with any of these at the moment. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... File tools/android/loading/sandwich_task_builder.py (right): https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:158: """Populate the a benchmark's pipeline from it's setup tasks. Populate benchmarking tasks from its setup tasks. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:161: subresource_discoverer: Name of a sub-resources discoverer. Name of the subresource discoverer. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:169: <runner_transformer_name> name/<subresource_discoverer>-metrics.csv rather: <runner_transformer_name>/<subresource_discoverer>-metrics.csv https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:172: depends on: common/<subresource_discoverer>-setup.wpr s/wpr/json/ https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:172: depends on: common/<subresource_discoverer>-setup.wpr small enhancement: depends on: common/<subresource_discoverer>-setup.json depends on: the end task saved by PopulateCommonPipelines() https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:175: The last task_manager.Task of the pipeline. task_manager.Task for <runner_transformer_name>/<subresource_discoverer>-metrics.csv
https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... File tools/android/loading/sandwich_task_builder.py (right): https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:162: runner_transformer: A closure that would be applied once to SandwichRunner On 2016/04/21 18:21:44, pasko wrote: > On 2016/04/21 09:32:46, gabadie wrote: > > On 2016/04/20 18:57:22, pasko wrote: > > > This is not a closure. > > > > > > "A function that takes an instance of SandwichRunner as parameter, would be > > > applied immediately before SandwichRunner.Run()" > > > > I insist on the closure. A function is a closure but with no mapping > associating > > variable names to ones defined in an enclosing scope (excluding the global > scope > > of course). > > > > Here the user may pass down a closure. > > As discussed offline, 'closure' can be confusing for people familiar with > chromium base/, where it means a callback without free arguments. So, let's > refer to it as a function. Done. https://codereview.chromium.org/1872313002/diff/120001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:187: "This recipe is not ready for multiple urls." On 2016/04/21 18:21:44, pasko wrote: > On 2016/04/21 09:32:46, gabadie wrote: > > On 2016/04/20 18:57:22, pasko wrote: > > > no need in this explanation, the assert is enough. You can put a > > TODO(gabadie): > > > Implement support for multiple URLs in this Task. > > > > Done. > > Really done? My bad. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:162: None to list all requested urls; On 2016/04/21 18:21:44, pasko wrote: > these rules are hard to remember, so the reader would have to always look up the > function definition when the function is called. > > More enum-like interface would be good: > > class _RequestOutcome: > All, ServedFromCache, NotServedFromCache = range(3) > > then: > > def _ListUrlRequests(trace, request_kind): > """... > > Args: > request_kind: _RequestOutcome indicating the subset of requests to output. > > then to call it: > > _ListUrlRequests(trace, _RequestOutcome.ServedFromCache) Done. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:173: if not request_event.protocol.startswith('http'): On 2016/04/21 18:21:44, pasko wrote: > This was not mentioned in the docstring. Does this filter out data requests? > Let's filter out data urls. This what this is for. > > I really prefer a blacklist. This would allow deciding what to do with other > protocols as they appear case-by-case. Having a blind whitelist for http/https > would make us silently produce wrong data in such cases. Done. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:181: def VerifyBenchmarkOutputDirectory(benchmark_setup_path, On 2016/04/21 18:21:44, pasko wrote: > Need to apply the same action as for ValidateCacheArchiveContent() - i.e. TODO > or raise exception or put status in the final CSV. Acknowledged. But I don't want to block sandwich workflow because of AJAX requests. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:216: def ListResourceUrls(benchmark_output_directory_path): On 2016/04/21 18:21:44, pasko wrote: > ReadSubresourceMapFromBenchmarkOutput(...) Second time you ask me for modification! Done. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:217: """Lists all requested URLs per navigated URLs On 2016/04/21 18:21:44, pasko wrote: > """Extracts a map URL-to-subresources for each navigation in benchmark > directory.""" Second time you ask me for modification! Done. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:251: def ValidateCacheArchiveContent(ref_urls, cache_archive_path): On 2016/04/21 18:21:44, pasko wrote: > Producing log messages on error is insufficient - easy to miss when running all > benchmarks. > > I see these options: > > 1. raise an exception and discard the whole Task scenario if cache content is > inconsistent. This is simplest to do, and probably good enough because other > benchmarks would probably be flawed and the data be useless. > > 2. Continue but put the result of validation in an intermediate .json and > propagate it to the final CSV. More involved. > > 3. Put a TODO for the next CL. > > I am OK with any of these at the moment. I don't want to block sandwich workflow because of AJAX requests. Adding TODO. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... File tools/android/loading/sandwich_task_builder.py (right): https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:158: """Populate the a benchmark's pipeline from it's setup tasks. On 2016/04/21 18:21:45, pasko wrote: > Populate benchmarking tasks from its setup tasks. Done. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:161: subresource_discoverer: Name of a sub-resources discoverer. On 2016/04/21 18:21:44, pasko wrote: > Name of the subresource discoverer. Done. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:169: <runner_transformer_name> name/<subresource_discoverer>-metrics.csv On 2016/04/21 18:21:45, pasko wrote: > rather: > <runner_transformer_name>/<subresource_discoverer>-metrics.csv Done. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:172: depends on: common/<subresource_discoverer>-setup.wpr On 2016/04/21 18:21:44, pasko wrote: > small enhancement: > depends on: common/<subresource_discoverer>-setup.json > depends on: the end task saved by PopulateCommonPipelines() Done. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:172: depends on: common/<subresource_discoverer>-setup.wpr On 2016/04/21 18:21:44, pasko wrote: > s/wpr/json/ Done. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_task_builder.py:175: The last task_manager.Task of the pipeline. On 2016/04/21 18:21:44, pasko wrote: > task_manager.Task for > <runner_transformer_name>/<subresource_discoverer>-metrics.csv Done.
lgtm with a minor change in docstring. Other suggestions are purely optional. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:216: def ListResourceUrls(benchmark_output_directory_path): On 2016/04/22 14:16:42, gabadie wrote: > On 2016/04/21 18:21:44, pasko wrote: > > ReadSubresourceMapFromBenchmarkOutput(...) > > Second time you ask me for modification! Done. Acknowledged. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:217: """Lists all requested URLs per navigated URLs On 2016/04/22 14:16:42, gabadie wrote: > On 2016/04/21 18:21:44, pasko wrote: > > """Extracts a map URL-to-subresources for each navigation in benchmark > > directory.""" > > Second time you ask me for modification! Done. I will keep asking for modifications as many times as needed. Thanks. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:251: def ValidateCacheArchiveContent(ref_urls, cache_archive_path): On 2016/04/22 14:16:42, gabadie wrote: > On 2016/04/21 18:21:44, pasko wrote: > > Producing log messages on error is insufficient - easy to miss when running > all > > benchmarks. > > > > I see these options: > > > > 1. raise an exception and discard the whole Task scenario if cache content is > > inconsistent. This is simplest to do, and probably good enough because other > > benchmarks would probably be flawed and the data be useless. > > > > 2. Continue but put the result of validation in an intermediate .json and > > propagate it to the final CSV. More involved. > > > > 3. Put a TODO for the next CL. > > > > I am OK with any of these at the moment. > > I don't want to block sandwich workflow because of AJAX > requests. Adding TODO. nit: In browser development the term XMLHttpRequest is more preferable to use, because AJAX is not carefully defined. https://codereview.chromium.org/1872313002/diff/200001/tools/android/loading/... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/200001/tools/android/loading/... tools/android/loading/sandwich_misc.py:164: trace: The trace. trace: (LoadingTrace) loading trace. https://codereview.chromium.org/1872313002/diff/200001/tools/android/loading/... tools/android/loading/sandwich_misc.py:176: assert request_event.protocol.startswith('http') I can remember you preferring to avoid asserts on things that are dependent on user input. Maybe raise Exception then? Up to you.
Thanks Egor for the review! Will rebase and land this CL once some other conflicting CLs will be landed. https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/160001/tools/android/loading/... tools/android/loading/sandwich_misc.py:251: def ValidateCacheArchiveContent(ref_urls, cache_archive_path): On 2016/04/25 13:29:06, pasko wrote: > On 2016/04/22 14:16:42, gabadie wrote: > > On 2016/04/21 18:21:44, pasko wrote: > > > Producing log messages on error is insufficient - easy to miss when running > > all > > > benchmarks. > > > > > > I see these options: > > > > > > 1. raise an exception and discard the whole Task scenario if cache content > is > > > inconsistent. This is simplest to do, and probably good enough because other > > > benchmarks would probably be flawed and the data be useless. > > > > > > 2. Continue but put the result of validation in an intermediate .json and > > > propagate it to the final CSV. More involved. > > > > > > 3. Put a TODO for the next CL. > > > > > > I am OK with any of these at the moment. > > > > I don't want to block sandwich workflow because of AJAX > > requests. Adding TODO. > > nit: In browser development the term XMLHttpRequest is more preferable to use, > because AJAX is not carefully defined. Acknowledged. https://codereview.chromium.org/1872313002/diff/200001/tools/android/loading/... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/200001/tools/android/loading/... tools/android/loading/sandwich_misc.py:164: trace: The trace. On 2016/04/25 13:29:06, pasko wrote: > trace: (LoadingTrace) loading trace. Done. https://codereview.chromium.org/1872313002/diff/200001/tools/android/loading/... tools/android/loading/sandwich_misc.py:176: assert request_event.protocol.startswith('http') On 2016/04/25 13:29:06, pasko wrote: > I can remember you preferring to avoid asserts on things that are dependent on > user input. Maybe raise Exception then? > > Up to you. Yes but here it is not an user input related check. The point of this assert is to internally break whenever we are facing an unexpected protocol that we need to understand to handle correctly rather than assuming it works like HTTP.
https://codereview.chromium.org/1872313002/diff/200001/tools/android/loading/... File tools/android/loading/sandwich_misc.py (right): https://codereview.chromium.org/1872313002/diff/200001/tools/android/loading/... tools/android/loading/sandwich_misc.py:176: assert request_event.protocol.startswith('http') On 2016/04/27 08:32:16, gabadie wrote: > On 2016/04/25 13:29:06, pasko wrote: > > I can remember you preferring to avoid asserts on things that are dependent on > > user input. Maybe raise Exception then? > > > > Up to you. > > Yes but here it is not an user input related check. I disagree, it depends on what a user puts into the job description. > The point of this assert is to internally break whenever we are facing an unexpected protocol > that we need to understand to handle correctly rather than assuming it works like HTTP. no problem, really. I prefer this rule: "Assertions should be used to check something that should never happen, while an exception should be used to check something that might happen." http://stackoverflow.com/questions/1957645/when-to-use-an-assertion-and-when-... It is minor in this case.
On 2016/04/27 08:32:16, gabadie wrote: > Thanks Egor for the review! > > Will rebase and land this CL once some other conflicting CLs will be landed. Sure. What are the conflicting changes? Do they need a review? Do they need a review from pasko@? :)
On 2016/04/27 08:52:11, pasko wrote: > On 2016/04/27 08:32:16, gabadie wrote: > > Thanks Egor for the review! > > > > Will rebase and land this CL once some other conflicting CLs will be landed. > > Sure. What are the conflicting changes? Do they need a review? Do they need a > review from pasko@? :) Done. Landing.
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org Link to the patchset: https://codereview.chromium.org/1872313002/#ps260001 (title: "Raises RuntimeError when encountering an unknown protocol")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1872313002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1872313002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== sandwich: Implement SandwichTaskBuilder Sandwich automation is moving to the task_manager API. This CL implements the SandwichTaskBuilder that builds the sandwich's task dependency graph. BUG=582080 ========== to ========== sandwich: Implement SandwichTaskBuilder Sandwich automation is moving to the task_manager API. This CL implements the SandwichTaskBuilder that builds the sandwich's task dependency graph. BUG=582080 Committed: https://crrev.com/e88c3b13065fac6f02ec54b95c3fe66ee89f0c37 Cr-Commit-Position: refs/heads/master@{#390048} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/e88c3b13065fac6f02ec54b95c3fe66ee89f0c37 Cr-Commit-Position: refs/heads/master@{#390048} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
