Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
545523002:
[Telemetry] Add capability for values to reference external files. (Closed)
Description[Telemetry] Add capability for values to reference external files.
This makes it so that values can optionally reference external files that provide trace-like details as to how it arose.
Patch Set 1 #Patch Set 2 : Hook up JSON + add tests #
Total comments: 2
Patch Set 3 : Better approach + more tests #
Total comments: 4
Patch Set 4 : New approach per Nat #
Total comments: 9
Patch Set 5 : Add more impl + unittests for trace #Patch Set 6 : minor typo fix #
Total comments: 2
Patch Set 7 : Hooked up JSON O.F., fleshed out TraceValue impl, added unittests. #Patch Set 8 : Address staticmethod nit #
Total comments: 11
Patch Set 9 : Address Nat's comments #
Total comments: 3
Patch Set 10 : Address Chris's comments + chart JSON html upload code #Patch Set 11 : painful rebase #Patch Set 12 : Attempt to make the bots happier #Patch Set 13 : Work around GPU triggered tests not knowing about trace_viewer #Messages
Total messages: 57 (18 generated)
eakuefner@chromium.org changed reviewers: + chrishenry@google.com, nednguyen@google.com
On 2014/09/05 00:12:22, eakuefner wrote: What is the merge policy for file?
https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/results/page_test_results.py (right): https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/results/page_test_results.py:110: files.update(set(v.files)) You may want to implement __hash__ and __eq__ for files for this to work correctly in the cases of different files object with same path. https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/value/__init__.py (right): https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/value/__init__.py:305: self.id = _next_file_id What happen if two File objects with a same path are created? Shouldn't they have the same id?
On 2014/09/05 02:59:16, nednguyen wrote: > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/results/page_test_results.py (right): > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/results/page_test_results.py:110: > files.update(set(v.files)) > You may want to implement __hash__ and __eq__ for files for this to work > correctly in the cases of different files object with same path. > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/value/__init__.py (right): > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/value/__init__.py:305: self.id = _next_file_id > What happen if two File objects with a same path are created? Shouldn't they > have the same id? I've taken a different approach that mitigates the problems you saw with the previous approach: instead, now, values keep track of a list of relative paths, and the dict of files that is created at serialization time is created with respect to normalized paths (thanks to slamm for pointing out that we should probably use normcase in addition to realpath). In addition, when a value is serialized, its files list, if it exists, is updated to be a list of IDs which point into the paths dict instead of being relative paths. This handles both 1. the problem of two values referring to the same file using different paths potentially causing multiple instances of the same file to be reported and 2. the problem of different paths that refer to the same file potentially having different IDs. As to the merge policy, we ought to make the values have sets of paths, and then union them when values are merged. What do you think about flattening the sets to sorted lists of IDs in serialization, since sets are not serializable? I'm going to upload a patch set to implement this but in the meantime I would appreciate any comments you would have on the new approach.
On 2014/09/05 22:53:36, eakuefner wrote: > On 2014/09/05 02:59:16, nednguyen wrote: > > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/results/page_test_results.py (right): > > > > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/results/page_test_results.py:110: > > files.update(set(v.files)) > > You may want to implement __hash__ and __eq__ for files for this to work > > correctly in the cases of different files object with same path. > > > > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/value/__init__.py (right): > > > > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/value/__init__.py:305: self.id = _next_file_id > > What happen if two File objects with a same path are created? Shouldn't they > > have the same id? > > I've taken a different approach that mitigates the problems you saw with the > previous approach: instead, now, values keep track of a list of relative paths, > and the dict of files that is created at serialization time is created with > respect to normalized paths (thanks to slamm for pointing out that we should > probably use normcase in addition to realpath). In addition, when a value is > serialized, its files list, if it exists, is updated to be a list of IDs which > point into the paths dict instead of being relative paths. This handles both 1. > the problem of two values referring to the same file using different paths > potentially causing multiple instances of the same file to be reported and 2. > the problem of different paths that refer to the same file potentially having > different IDs. > > As to the merge policy, we ought to make the values have sets of paths, and then > union them when values are merged. What do you think about flattening the sets > to sorted lists of IDs in serialization, since sets are not serializable? I'm > going to upload a patch set to implement this but in the meantime I would > appreciate any comments you would have on the new approach. Wait, actually, as far as merging goes, we can just concatenate the lists, and then at serialization time, convert to set, and then output sorted list of IDs. This follows this approach of deferring deduplication until serialization time, when it actually matters.
so i hadn't envisioned all values having associated files. i more thought the base class could have an associated file, but most of the values wouldn't use that capability. we'd just teach the tracevalue class to do it directly.
On 2014/09/05 22:55:18, eakuefner wrote: > On 2014/09/05 22:53:36, eakuefner wrote: > > On 2014/09/05 02:59:16, nednguyen wrote: > > > > > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > > > File tools/telemetry/telemetry/results/page_test_results.py (right): > > > > > > > > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > > > tools/telemetry/telemetry/results/page_test_results.py:110: > > > files.update(set(v.files)) > > > You may want to implement __hash__ and __eq__ for files for this to work > > > correctly in the cases of different files object with same path. > > > > > > > > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > > > File tools/telemetry/telemetry/value/__init__.py (right): > > > > > > > > > https://codereview.chromium.org/545523002/diff/20001/tools/telemetry/telemetr... > > > tools/telemetry/telemetry/value/__init__.py:305: self.id = _next_file_id > > > What happen if two File objects with a same path are created? Shouldn't they > > > have the same id? > > > > I've taken a different approach that mitigates the problems you saw with the > > previous approach: instead, now, values keep track of a list of relative > paths, > > and the dict of files that is created at serialization time is created with > > respect to normalized paths (thanks to slamm for pointing out that we should > > probably use normcase in addition to realpath). In addition, when a value is > > serialized, its files list, if it exists, is updated to be a list of IDs which > > point into the paths dict instead of being relative paths. This handles both > 1. > > the problem of two values referring to the same file using different paths > > potentially causing multiple instances of the same file to be reported and 2. > > the problem of different paths that refer to the same file potentially having > > different IDs. > > > > As to the merge policy, we ought to make the values have sets of paths, and > then > > union them when values are merged. What do you think about flattening the sets > > to sorted lists of IDs in serialization, since sets are not serializable? I'm > > going to upload a patch set to implement this but in the meantime I would > > appreciate any comments you would have on the new approach. I think the new approach is better. To figure out the path ids, you first need to have all the paths available. Using hidden global to solve this isn't great since it makes object states depend on which instances are available. > > Wait, actually, as far as merging goes, we can just concatenate the lists, and > then at serialization time, convert to set, and then output sorted list of IDs. > This follows this approach of deferring deduplication until serialization time, > when it actually matters. I don't think it's critical to it to have path's infos for the merged values. Since merged values are supposed to be a non-detailed view of the data, so things like file paths are details that doesn't necessarily belong there. In addition, how helpful is it to show that the merged list of frame times [13, 12, 11, 13] across 3 pages are associated with files [foo.json, bar.json]?
https://codereview.chromium.org/545523002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/results/json_output_formatter.py (right): https://codereview.chromium.org/545523002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/results/json_output_formatter.py:40: else: You can just keep the initialization of results_dict as the current states, and then swap 'paths' : [....] with 'path_ids': [....] for pages' values, summary_values, and per_page_values. It seems to make the code flow easier to follow, e.g: .. if paths: _ReplacePathsWithPathIds(results_dict['summary_values'], paths_to_ids) _ReplacePathsWithPathIds(results_dict['summary_values'], paths_to_ids) https://codereview.chromium.org/545523002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/results/json_output_formatter.py:73: new_dict['paths'] = new_paths I think 'path_ids' is the right term. You can just remove the 'paths' and replace with 'path_ids'. https://codereview.chromium.org/545523002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/value/__init__.py (right): https://codereview.chromium.org/545523002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/value/__init__.py:59: paths: A list of paths corresponding to files associated with this value. What are paths? Are they strings, or are they instances of some classes? In addition, what does it mean if some paths are relative? i.e: which file location does ./foo.txt refers too? Do paths need to exist? It seems to me that having a Path class like your previous patch that answers these questions and raise corresponding exceptions in case of invalid values is better the strings representation.
nduca@chromium.org changed reviewers: + nduca@chromium.org
please make a FileHandle object that is a TempFile or constructed-from-filepath-but-doesnt-delete it. put it in util.
https://codereview.chromium.org/545523002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/value/__init__.py (left): https://codereview.chromium.org/545523002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/value/__init__.py:188: d['page_id'] = self.page.id How about ""Returns file handle... or none.""" def GetAssociatedFileHandle(self): return None
On 2014/09/08 18:53:02, nduca wrote: > https://codereview.chromium.org/545523002/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/value/__init__.py (left): > > https://codereview.chromium.org/545523002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/value/__init__.py:188: d['page_id'] = self.page.id > How about > > ""Returns file handle... or none.""" > def GetAssociatedFileHandle(self): > return None Nat PTAL -- I've written FileHandle and sketched out TraceValue and I want comments on the overall approach. Notable remaining issues include: linter complains about unimplemented buildbot methods (may not matter), what's the merge policy, and what do we do about serialization. I'd appreciate further guidance on how/if to proceed with TraceValue as well. For serialization, I'm inclined to simply insert a 'file_path' key in the dict for any value who has a file, at output time. Since JsonOutputFormatter knows its output_stream, I can ask for output_stream.name and calculate the relpath -- presently JsonOutputFormatter always outputs to a file, which is results.json in the current directory by default.
https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/util/file_handle.py (right): https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/util/file_handle.py:10: def __init__(self, handle): i didn't mean for it to own an open file object. as you've written it, the named temporary file would get gc'd and the file would get killed pretty sure. it should have self._path and self._tempfile, one of each, which accessors for the absolute_filename. But it shouldn't have an open file... if they want to open it, they can open the file themselves. https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/value/__init__.py (right): https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/value/__init__.py:278: return self._file_handle er, return None? The subclasses should override and do the real work. https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/value/trace.py (right): https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/value/trace.py:12: def __init__(self, page, name, units, trace_data, important=False, er, you should round this out and think this through a bit harder. traces don't have units. they're traces. so dont require them to pass in a unit. s/trace_data/timeline_data/ and specify what the timeline data type is in the constructor, you should write the timeline data to a temporary file: here in this file tf = NamedTemporaryFile() timleine_data.Serialize(tf) this._file_handle = file_handle.FromTempFile(tf) GetAssociatedFileHandle() then just returns the file handle. MergeSamePage would probably be to take the first of the list. MergeAcross would be to return None.
https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/util/file_handle.py (right): https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/util/file_handle.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. telemetry.io instead? (Sorry, I don't like util package.) https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/value/trace.py (right): https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/value/trace.py:15: self._file_handle = file_handle.FileHandle.FromTempFile() Huh, why are we writing to temp file? Don't we want this to be available after the benchmark is done too? I'm confused. Also, why are we not writing anything to it? https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/value/trace.py:31: def WriteTraceToFile(handle, trace_data): Where is this being called from?
https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/util/file_handle.py (right): https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/util/file_handle.py:10: def __init__(self, handle): On 2014/09/09 01:39:06, nduca wrote: > i didn't mean for it to own an open file object. as you've written it, the named > temporary file would get gc'd and the file would get killed pretty sure. > > it should have self._path and self._tempfile, one of each, which accessors for > the absolute_filename. But it shouldn't have an open file... if they want to > open it, they can open the file themselves. It is not clear to me why we shouldn't keep an open handle in here. Tempfiles are open when constructed, so while we could build a tempfile, then close it so that it can be opened later by whoever is using FileHandle, that smells to me. It seems like a FileHandle should represent either a file corresponding to a given path, or a temporary file, and so I don't see why we should have separate fields for path and tempfile. I'm not sure that FromTempFile should actually take an argument since this seems contrary to what we actually want it to do: if all the user wants is to be able to construct a FileHandle object from a handle, they can call the constructor. It turns out that tempfiles are actually deleted not when GC happens, but when they are closed, by default. The latest patch set passes delete=False to the NamedTemporaryFile constructor so that the created files can actually be read later. % the open/closed issue I think the FileHandle in the current patch set is reasonably close given the points I've raised. I think having it support __enter__ and __exit__ in order so that users can use 'with' to delegate open/close management to the FileHandle object itself is not a bad idea, since currently Serialize methods have to call close() on their own. Please let me know what you think. https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/value/__init__.py (right): https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/value/__init__.py:278: return self._file_handle On 2014/09/09 01:39:06, nduca wrote: > er, return None? The subclasses should override and do the real work. Done. https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/value/trace.py (right): https://codereview.chromium.org/545523002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/value/trace.py:12: def __init__(self, page, name, units, trace_data, important=False, On 2014/09/09 01:39:06, nduca wrote: > er, you should round this out and think this through a bit harder. > > traces don't have units. they're traces. so dont require them to pass in a unit. > > s/trace_data/timeline_data/ and specify what the timeline data type is > > in the constructor, you should write the timeline data to a temporary file: > here in this file > > tf = NamedTemporaryFile() > timleine_data.Serialize(tf) > this._file_handle = file_handle.FromTempFile(tf) > > GetAssociatedFileHandle() then just returns the file handle. > > MergeSamePage would probably be to take the first of the list. > MergeAcross would be to return None. > I added more implementation details and a couple unittests. There is still more work to be done but this file handle thing is still a pretty major sticking point (see comment reply on FileHandle).
https://codereview.chromium.org/545523002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/util/file_handle.py (right): https://codereview.chromium.org/545523002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/util/file_handle.py:9: class FileHandle(object): as i mentioned on chat this still wont work. FileHandle(TempFile()) should delete the tempfile when both the handle and the tempfile have been gc'd FileHandle("foo.json") should not delete Sometimes reviewers give you feedback. Its great to spend some time learning why, but when a reviewer gives you really crisp interface as I did for a side class [twice already] it might be worth coding it up and just doing it and then learning about it in free time. Right now, this whole patch is kinda bottlenecking iterating on this 15 line class. https://codereview.chromium.org/545523002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/util/file_handle.py:27: def FromTempFile(): this is a staticmethod
are we hooking it into the json object in this patch?
On 2014/09/12 01:45:20, nduca wrote: > are we hooking it into the json object in this patch? I would like to, yes. As discussed in chat there will be some subtleties about copying/moving and so forth that I'll have to address.
Patchset #7 (id:120001) has been deleted
Patchset #8 (id:160001) has been deleted
Finally got an implementation fleshed out. PTAL.
https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... File tools/telemetry/telemetry/util/file_handle.py (right): https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/util/file_handle.py:31: return self._id I am still not convinced that we need the id for this API. As discussed before, the logic of assigning these ids can be delayed until all the files are about to be written to json. https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... File tools/telemetry/telemetry/value/trace.py (right): https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/value/trace.py:50: def MergeLikeValuesFromSamePage(cls, values): This should explode if these values are not the same.
lgtm https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... File tools/telemetry/telemetry/results/json_output_formatter.py (right): https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/results/json_output_formatter.py:34: trace_values = _AllTraceValues(page_test_results) er, why not just all filehandles? e.g. you just do v for v in values if v.GetFileHandle()? https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... File tools/telemetry/telemetry/util/file_handle.py (right): https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/util/file_handle.py:23: self._path = path self._path -> self._absolute_path https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/util/file_handle.py:31: return self._id as coded, this i think this is the right approach. i agree with what ethan has done here. if ids are assigned at output time, then Value.AsDict() needs to have access to this id mapping. We've got precidence for this approach: this is how pages work https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/util/file_handle.py:33: def GetRelPath(self, path='.'): i think path=None is the right behavior here https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/util/file_handle.py:33: def GetRelPath(self, path='.'): i think path=None is the right behavior here rename path->start to match os.path.relpath https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/util/file_handle.py:66: return FileHandle(None, path) pass in os.path.abspath(path) https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... File tools/telemetry/telemetry/value/trace.py (right): https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/value/trace.py:17: self._file_handle = file_handle.FileHandle.FromTempFile(tf) swap line 17 and 18 https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/value/trace.py:18: timeline_data.Serialize(tf) tf = ... timelineD_ata.Serialize(tf) tf2 = NamedTemporaryFile() import telemetry.web_components from trace_viewer.build import trace2html trace2html.WriteHTMLForTracesToFile([tf.name], tf2) this._file_handle = file_handle.BlahBlah(tf2) https://codereview.chromium.org/545523002/diff/180001/tools/telemetry/telemet... tools/telemetry/telemetry/value/trace.py:50: def MergeLikeValuesFromSamePage(cls, values): this is fine, for now. There is an even better solution here but its follow up: the trace file should be html. Then once we have that, we can make a MultiTraceViewer. # TODO(eakuenfer): Implement a MultiTraceValue
lgtm https://codereview.chromium.org/545523002/diff/200001/tools/telemetry/telemet... File tools/telemetry/telemetry/util/file_handle.py (right): https://codereview.chromium.org/545523002/diff/200001/tools/telemetry/telemet... tools/telemetry/telemetry/util/file_handle.py:17: tempfile: An instance of a temporary file object. You said tempfile here but tf in the actual signature. https://codereview.chromium.org/545523002/diff/200001/tools/telemetry/telemet... File tools/telemetry/telemetry/value/trace.py (right): https://codereview.chromium.org/545523002/diff/200001/tools/telemetry/telemet... tools/telemetry/telemetry/value/trace.py:14: def __init__(self, page, name, timeline_data, important=False, pydoc https://codereview.chromium.org/545523002/diff/200001/tools/telemetry/telemet... tools/telemetry/telemetry/value/trace.py:60: # TODO(eakuefner): Implement a MultiTraceValue. What does this mean? Is this just a list of TraceValue?
The CQ bit was checked by eakuefner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545523002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/60914) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by eakuefner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545523002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
The CQ bit was checked by eakuefner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545523002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
The CQ bit was checked by eakuefner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545523002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu_retina_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
The CQ bit was checked by eakuefner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545523002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545523002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sullivan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545523002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
I'm taking over this patch. The unit test fixes are easy, but I have questions about other parts of the patch. Such as, what is FileHandle and why do we need it? Seems like a lot of code for something we shouldn't be doing anyway - assigning global IDs.
I don't know about the details of the patch, but at a very high level, as a user of this feature I would really like to be able to: * Have trace cloud storage urls available in the chartjson that gets sent to perf dashboard for chromium.perf buildbots (still working on the buildbot side of this) * Have trace cloud storage urls in the output of perf tryjobs on tryserver.chromium.perf, so I can add a buildbot annotation that links to them. It looks like with the way the patch is now, I'd need to switch the perf tryjobs to use chartjson to do this, since the upload is in chart_json_output_formatter, right? On Thu, Oct 2, 2014 at 7:31 PM, <dtu@chromium.org> wrote: > I'm taking over this patch. The unit test fixes are easy, but I have > questions > about other parts of the patch. Such as, what is FileHandle and why do we > need > it? Seems like a lot of code for something we shouldn't be doing anyway - > assigning global IDs. > > https://codereview.chromium.org/545523002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/03 13:47:04, chromium-reviews wrote: > I don't know about the details of the patch, but at a very high level, as a > user of this feature I would really like to be able to: > > * Have trace cloud storage urls available in the chartjson that gets sent > to perf dashboard for chromium.perf buildbots (still working on the > buildbot side of this) > * Have trace cloud storage urls in the output of perf tryjobs on > tryserver.chromium.perf, so I can add a buildbot annotation that links to > them. It looks like with the way the patch is now, I'd need to switch the > perf tryjobs to use chartjson to do this, since the upload is in > chart_json_output_formatter, right? > > On Thu, Oct 2, 2014 at 7:31 PM, <mailto:dtu@chromium.org> wrote: > > > I'm taking over this patch. The unit test fixes are easy, but I have > > questions > > about other parts of the patch. Such as, what is FileHandle and why do we > > need > > it? Seems like a lot of code for something we shouldn't be doing anyway - > > assigning global IDs. > > > > https://codereview.chromium.org/545523002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Chatted with Dave offline. Dave will take over the devtool_backend refactoring and I will take ownership of landing this.
On 2014/10/15 22:56:36, nednguyen wrote: > On 2014/10/03 13:47:04, chromium-reviews wrote: > > I don't know about the details of the patch, but at a very high level, as a > > user of this feature I would really like to be able to: > > > > * Have trace cloud storage urls available in the chartjson that gets sent > > to perf dashboard for chromium.perf buildbots (still working on the > > buildbot side of this) > > * Have trace cloud storage urls in the output of perf tryjobs on > > tryserver.chromium.perf, so I can add a buildbot annotation that links to > > them. It looks like with the way the patch is now, I'd need to switch the > > perf tryjobs to use chartjson to do this, since the upload is in > > chart_json_output_formatter, right? > > > > On Thu, Oct 2, 2014 at 7:31 PM, <mailto:dtu@chromium.org> wrote: > > > > > I'm taking over this patch. The unit test fixes are easy, but I have > > > questions > > > about other parts of the patch. Such as, what is FileHandle and why do we > > > need > > > it? Seems like a lot of code for something we shouldn't be doing anyway - > > > assigning global IDs. > > > > > > https://codereview.chromium.org/545523002/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Chatted with Dave offline. Dave will take over the devtool_backend refactoring > and I will take ownership of landing this. A few things about FileHandle, off the top of my head: * We need to assign an id, because the json results need to be able to point to the files * We need to be able to track of opened FileHandle and close them * It is useful for testing, to inject fake FileHandle |
