Expose GPU information to Telemetry.

Expose GPU information to Telemetry.

The GPU on the system must be known to Telemetry in order for the test
expectations to be applied properly. Added SystemInfo domain to the
DevTools protocol and GetGPUInfo method to Telemetry's Browser class.
Added GPUInfo and GPUDevice classes to telemetry.core. Wrote and ran tests
for all new code.

BUG=245741
R=dtu@chromium.org,zmo@chromium.org,pfeldman@chromium.org,nduca@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217784

[Ken Russell (switch to Gerrit), 2013-08-02 00:55:11]

Please review.

dtu => tools/telemetry (and everything)
jochen => content/browser/renderer_host, content/renderer
palmer => content/common/gpu/gpu_messages.h IPC review
zmo => gpu/config

[Zhenyao Mo, 2013-08-02 01:07:47]

On 2013/08/02 00:55:11, Ken Russell wrote:
> Please review.
> 
> dtu => tools/telemetry (and everything)
> jochen => content/browser/renderer_host, content/renderer
> palmer => content/common/gpu/gpu_messages.h IPC review
> zmo => gpu/config

gpu/config LGTM

Thanks for implementing this

[dtu, 2013-08-02 01:35:18]

Looks great! Only nits.

https://codereview.chromium.org/21682002/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/gpu_device.py (right):

https://codereview.chromium.org/21682002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/gpu_device.py:41: return
GPUDevice(attrs['vendor_id'], attrs['device_id'],
nit: cls in place of GPUDevice

https://codereview.chromium.org/21682002/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/gpu_info.py (right):

https://codereview.chromium.org/21682002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/gpu_info.py:10: def __init__(self, attrs):
nit: Make this a FromDict classmethod.

https://codereview.chromium.org/21682002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/gpu_info.py:11: """Parses itself from a
dictionary of attributes supplied by the
Google Python style for docstrings: One-line summary, followed by a blank line,
followed by longer information.

https://codereview.chromium.org/21682002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/gpu_info.py:53: '\' in dictionary')
nit: raise Exception('Missing required attribute "%s" in dictionary' % a)

https://codereview.chromium.org/21682002/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/tab_unittest.py (right):

https://codereview.chromium.org/21682002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/tab_unittest.py:101: self.assertIsInstance(info,
gpu_info.GPUInfo)
assertIsInstance is Python 2.7 and the bots are on 2.6 :(

[dtu, 2013-08-02 01:35:22]

lgtm

Looks great! Only nits.

[jochen (gone - plz use gerrit), 2013-08-02 07:39:35]

lgtm

[palmer, 2013-08-02 20:26:36]

LGTM

https://codereview.chromium.org/21682002/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/gpu_message_filter.cc (right):

https://codereview.chromium.org/21682002/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/gpu_message_filter.cc:93:
IPC_MESSAGE_HANDLER(GpuHostMsg_GetGPUInfo,
Nit: This could fit on one line?

[nduca, 2013-08-04 20:12:15]

woah this needs more work! more comments to follow once i've figured out how to
explain.

[nduca, 2013-08-04 20:32:06]

So the core issue is that any new API area added to telemetry/core must first
pass the test "will this work on other browsers." For example, IE. We need to be
incredibly dillegent when adding apis to telemetry.

Now, I know Ken in particular, you have a very important goal and just want to
make headway. One way to do this without getting caught in review hell caused by
me is to not touch the telemetry/core module at all. Rather, you could make make
a GpuInfoReader class in the gpu test folder that adds the right startup
arguments to the browser, and then retrieves it. Then you have common code for
getting this stuff, but you dont run afoul of the "core telemetry always works
everywhere" dictum. That might be the most expedient for your use case, at least
for now.


If you want to go down the road of this being in core telemetry, which I think
is the right thing though perhaps only when time constraints permit,  then we
have to a few basic formative questions:
1. will this be something that other browser might eventually support?
2. is it a tab-specific API, a browser-specific api, or a platform specific API?

Now, for #2, it seems to me that GPU info is either browser-specific or
platform-specific. Platform-specific APIs are typically things we fill out
without having to talk to chrome, e.g. by doing stuff like ps ax, so I think
that's not quite the right fit in this case since we have to run a ton of C++ to
get the gpu info. Thus, it seems to me that GPU info is a browser-level call.

The other piece here to consider is how other browsers might divulge GPU info.
Telemetry works on WebDriver-compatible browsers now, for instance, e.g. IE. Its
likely that they'd expose it via an extension, right? Or perhaps its just too
soon to know... but, in that case, browser level seems like the most
conservative choice. We could always behind the scenes of that call load an
extension on a particular browser in order to retrieve GPU info.

Now, in chrome, a better way to implement gpu info accesss as a browser level
operation is to add it to the devtools browser protocol ---
content/browser/devtools has code that maps  websocket messages to
:9222/json/browser to c++ code handlers. You could add a getGpuInfo method and
voila! No gpu_benchmarking_extension dependency.

Finally there's the discussion of whether the GpuInfo class is somethign that
other vendors could fully fill in. And, for that matter, what some of the fields
mean. lenovo_dcute and amd_switchable... would firefox call these things the
same name? Perhaps a better solution to that api is to define some mandatory
fields which have complete docstrings and are backed by real python fields
instead of set via setattr magic, and then have some additional_data field htat
has the other things that are perhaps more specific to chrome.

Let me know if you want to talk Monday about any of this. I think you could get
this into landable shape in telemetry by:
1. moving the method to devtools browser and out of the benchmarking extension
2. adding more strong naming/typing and required/optionality discusison to the
GpuInfo class

[nduca, 2013-08-06 22:14:12]

+tonyg for systeminfo angle

[Ken Russell (switch to Gerrit), 2013-08-07 01:42:12]

Restructured as API on Telemetry's Browser object on feedback from nduca. Please
re-review.

pfeldman: please review content/browser/devtools/ changes. Decision to add new
SystemInfo domain was made after discussion with nduca and tonyg.

[pfeldman, 2013-08-07 13:01:58]

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.cc (right):

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:18: const char kGPU[] =
"gpu";
How is this information going to be used? Do you intend to make vendorId,
deviceId, etc. public and commit to its maintenance? If so, we might need to
come up with browser_protocol.json (like protocol.json) that would describe the
scheme.

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:19: const char
kVendorId[] = "vendor_id";
Please use camelCase for property names.

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.h (right):

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.h:14: public
DevToolsProtocol::Handler {
Weird indent.

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.h:20:
scoped_ptr<DevToolsProtocol::Response> OnGetGPUInfo(
You'll need to rebaseline it to scoped_refptr<Response> and
scoped_refptr<Command>

[Ken Russell (switch to Gerrit), 2013-08-08 02:42:36]

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.cc (right):

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:18: const char kGPU[] =
"gpu";
On 2013/08/07 13:01:58, pfeldman wrote:
> How is this information going to be used? Do you intend to make vendorId,
> deviceId, etc. public and commit to its maintenance? If so, we might need to
> come up with browser_protocol.json (like protocol.json) that would describe
the
> scheme.

The CL description describes how it's going to be used. Currently I have no plan
to document and publicize this information. It is required in order to cut over
many GPU tests to the Telemetry harness, which is a top priority for our group.

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:19: const char
kVendorId[] = "vendor_id";
On 2013/08/07 13:01:58, pfeldman wrote:
> Please use camelCase for property names.

Is this strictly necessary? Doing so is going to require a bunch of boilerplate
conversion code because Python's naming convention differs from JavaScript's.
The only goal here is to expose these fields to Python. JavaScript is a
necessary intermediary.

[nduca, 2013-08-08 02:45:17]

> The CL description describes how it's going to be used. Currently I have no
plan
> to document and publicize this information. It is required in order to cut
over
> many GPU tests to the Telemetry harness, which is a top priority for our
group.
Our deal with Pavel is that when we add stuff here, we find a way to maintain it
for ever. It, like diamonds, is for life. Thats why I had pushed you also in the
telemetry to have a core set of types that you expose and optional ones. Can you
apply this in the protocol as well?

> Is this strictly necessary? Doing so is going to require a bunch of
boilerplate
> conversion code because Python's naming convention differs from JavaScript's.
> The only goal here is to expose these fields to Python. JavaScript is a
> necessary intermediary.
Yeah we (python telemetry) aren't the only client of devtools. We should stick
to the convention of devtools.

[Ken Russell (switch to Gerrit), 2013-08-14 00:54:40]

A separate Blink CL has been uploaded to address the concern of documenting the
addition of the SystemInfo domain to the DevTools protocol. All other review
feedback has been addressed. Please re-review.

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.cc (right):

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:18: const char kGPU[] =
"gpu";
On 2013/08/07 13:01:58, pfeldman wrote:
> How is this information going to be used? Do you intend to make vendorId,
> deviceId, etc. public and commit to its maintenance? If so, we might need to
> come up with browser_protocol.json (like protocol.json) that would describe
the
> scheme.

Per discussion above, I've uploaded https://codereview.chromium.org/22980005 to
document the new methods and types in the SystemInfo domain. It seemed that it
fit properly within the existing protocol.json along other domains such as
Tracing.

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:19: const char
kVendorId[] = "vendor_id";
On 2013/08/07 13:01:58, pfeldman wrote:
> Please use camelCase for property names.

Done. Also modified GPUInfo::EnumerateFields in gpu_info.cc to use camelCase. A
new helper class in Telemetry converts to the underscore naming convention used
throughout its Python code.

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.h (right):

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.h:14: public
DevToolsProtocol::Handler {
On 2013/08/07 13:01:58, pfeldman wrote:
> Weird indent.

Done.

https://codereview.chromium.org/21682002/diff/26001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.h:20:
scoped_ptr<DevToolsProtocol::Response> OnGetGPUInfo(
On 2013/08/07 13:01:58, pfeldman wrote:
> You'll need to rebaseline it to scoped_refptr<Response> and
> scoped_refptr<Command>

Done.

[Ken Russell (switch to Gerrit), 2013-08-14 07:27:23]

pfeldman, yurys: please review this during your work day today. Thank you.

[pfeldman, 2013-08-14 09:18:19]

https://codereview.chromium.org/21682002/diff/49001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.cc (right):

https://codereview.chromium.org/21682002/diff/49001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:23: const char
kSecondaryGPUs[] = "secondaryGpus";
I don't see neither aux gpu attributes nor machine name declared in the schema.
Do they exit? I'd rather return plain list of what you have and declare machine
/ gpu attrs when you have them.

https://codereview.chromium.org/21682002/diff/49001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:31:
dictionary_->SetDouble(name, value);
I don't see these in the schema.

https://codereview.chromium.org/21682002/diff/49001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:62: if
(!dictionary_->HasKey(kGPU)) {
This code would go away if you simply enumerate the devices.

[Ken Russell (switch to Gerrit), 2013-08-14 19:33:15]

Per pfeldman's feedback, added SystemInfo class to Telemetry and moved
machine_model and gpu (GPUInfo object) there. Changed API to
SystemInfo.getInfo().

Changed and simplified GPUInfo::Enumerator. Majority of attributes now properly
go in auxAttributes object per protocol description in
https://codereview.chromium.org/22980005/ .

Simplified error checking on Python side. Added more tests.

Please re-review.

https://codereview.chromium.org/21682002/diff/49001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.cc (right):

https://codereview.chromium.org/21682002/diff/49001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:23: const char
kSecondaryGPUs[] = "secondaryGpus";
On 2013/08/14 09:18:20, pfeldman wrote:
> I don't see neither aux gpu attributes nor machine name declared in the
schema.
> Do they exit? I'd rather return plain list of what you have and declare
machine
> / gpu attrs when you have them.

Sorry, this was a mistake on my part. The code's been fixed to conform to the
protocol.

https://codereview.chromium.org/21682002/diff/49001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:31:
dictionary_->SetDouble(name, value);
On 2013/08/14 09:18:20, pfeldman wrote:
> I don't see these in the schema.

Fixed.

https://codereview.chromium.org/21682002/diff/49001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:62: if
(!dictionary_->HasKey(kGPU)) {
On 2013/08/14 09:18:20, pfeldman wrote:
> This code would go away if you simply enumerate the devices.

Restructured so (nearly) all the enumeration code is in gpu_info.cc.

[nduca, 2013-08-14 19:53:36]

telemetry part of this looks really solid. nearly lg but nit around the backend
object...

https://codereview.chromium.org/21682002/diff/58001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/system_info_backend.py (right):

https://codereview.chromium.org/21682002/diff/58001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/system_info_backend.py:15: class
SystemInfoBackend(object):
hmm looks like most of this code is just to establish/comm with the browser
channel. maybe we can move that to the browser backend direclty, and have
methods like _SyncBrowserRequest(), and _CloseBrowserWebSocket() on it, and have
this and the tracing backend just call up to there.

[pfeldman, 2013-08-14 19:59:02]

https://codereview.chromium.org/21682002/diff/58001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.cc (right):

https://codereview.chromium.org/21682002/diff/58001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:27: , root_(dictionary)
Use trailing comma as per style guidelines.

https://codereview.chromium.org/21682002/diff/58001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:39:
current_->SetString(name, value);
Filling in gpu data this way does not guarantee that there will be vendorId,
deviceId, etc. You should explicitly set these properties in the handler code.
The only place you are allowed reflective enumeration is for "auxAttributes".

https://codereview.chromium.org/21682002/diff/58001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:102: bool found =
gpu_dict->Remove(kMachineModel, &machine_model);
nit: I'd call it modelName

[pfeldman, 2013-08-14 20:01:41]

https://codereview.chromium.org/21682002/diff/58001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.cc (right):

https://codereview.chromium.org/21682002/diff/58001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:39:
current_->SetString(name, value);
Previous iteration was properly filling in kDeviceString and such, so you are
regressing it here.

[Ken Russell (switch to Gerrit), 2013-08-14 22:47:56]

https://codereview.chromium.org/21682002/diff/58001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/system_info_backend.py (right):

https://codereview.chromium.org/21682002/diff/58001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/system_info_backend.py:15: class
SystemInfoBackend(object):
On 2013/08/14 19:53:36, nduca wrote:
> hmm looks like most of this code is just to establish/comm with the browser
> channel. maybe we can move that to the browser backend direclty, and have
> methods like _SyncBrowserRequest(), and _CloseBrowserWebSocket() on it, and
have
> this and the tracing backend just call up to there.

The tracing backend requires its own websocket connection to the browser because
it uses it in multiple threads. The best that could be done would be to refactor
common code between tracing_backend.py and system_info_backend.py into a base
class.

[Ken Russell (switch to Gerrit), 2013-08-14 23:18:07]

Moved browser_protocol.json into this CL.

Made enumeration explicit in DevToolsSystemInfoHandler.

Renamed fields.

Refactored common code between TracingBackend and SystemInfoBackend into
WebSocketBrowserConnection base class.

https://codereview.chromium.org/21682002/diff/58001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.cc (right):

https://codereview.chromium.org/21682002/diff/58001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:27: , root_(dictionary)
On 2013/08/14 19:59:03, pfeldman wrote:
> Use trailing comma as per style guidelines.

Fixed.

https://codereview.chromium.org/21682002/diff/58001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:39:
current_->SetString(name, value);
On 2013/08/14 20:01:41, pfeldman wrote:
> Previous iteration was properly filling in kDeviceString and such, so you are
> regressing it here.

Explicitly added all required fields in this file and changed enumerator to only
traverse auxiliary attributes.

https://codereview.chromium.org/21682002/diff/58001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:102: bool found =
gpu_dict->Remove(kMachineModel, &machine_model);
On 2013/08/14 19:59:03, pfeldman wrote:
> nit: I'd call it modelName

Done.

[Ken Russell (switch to Gerrit), 2013-08-14 23:22:19]

piman: please review content_browser.gypi change.

[nduca, 2013-08-14 23:40:42]

telemetry/ lgtm. could you perhaps make the connection be a member of both
backends instead of subclassing? make close & sync public members on the
connection? You'll have to put a little bit of shim code for the backend close
method to call close on the connection too, i guess.

thanks so much for this cleanup.

[Ken Russell (switch to Gerrit), 2013-08-15 01:35:10]

Refactored more code into WebSocketBrowserConnection and made it a helper object
rather than a base class.

Reran all tests.

[piman, 2013-08-15 02:50:21]

lgtm

[pfeldman, 2013-08-15 05:49:46]

devtools lgtm

https://codereview.chromium.org/21682002/diff/72001/content/browser/devtools/...
File content/browser/devtools/devtools_system_info_handler.cc (right):

https://codereview.chromium.org/21682002/diff/72001/content/browser/devtools/...
content/browser/devtools/devtools_system_info_handler.cc:107: for (size_t ii =
0; ii < gpu_info.secondary_gpus.size(); ++ii) {
nit: prefer iterator while iterating stl collections.

[commit-bot: I haz the power, 2013-08-15 09:20:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/21682002/72001

[commit-bot: I haz the power, 2013-08-15 11:37:47]

Change committed as 217784