chrome power profiler chrome side changes

chrome power profiler chrome side changes
BUG=None

[pfeldman, 2014-01-09 11:38:57]

https://codereview.chromium.org/106223002/diff/1/content/browser/browser_proc...
File content/browser/browser_process_sub_thread.cc (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/browser_proc...
content/browser/browser_process_sub_thread.cc:47:
power_profiler_service_.reset(new PowerProfilerService);
You should either use lazy_init, singleton or BrowserContextKeyedService
depending on how many instances you want to have, deletion pattern and
threading. Not manual initialization like this.

https://codereview.chromium.org/106223002/diff/1/content/browser/browser_thre...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/browser_thre...
content/browser/browser_thread_impl.cc:32: "Chrome_PowerProfilerThread", //
POWER_PROFILER
Why do you want a named thread for power profiler? Could we get away with an ad
hock one?

https://codereview.chromium.org/106223002/diff/1/content/browser/devtools/dev...
File content/browser/devtools/devtools_frontend_host.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/devtools/dev...
content/browser/devtools/devtools_frontend_host.h:47: void
OnPowerProfileStatus();
This should be a part of devtools_agent_host.h

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_event.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_event.h:13: struct PowerEvent {
You can define it within the helper.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_profiler_helper.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_helper.h:15: class
PowerProfilerHelper : public base::RefCounted<PowerProfilerHelper>{
"Helper" is not a useful suffix. What does it do?

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_helper.h:22: // reset resolution
of power profile driver.
Start with capital letters.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_helper.h:27: // data is the buffer
to store power event,
|data|

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_profiler_helper_dummy.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_helper_dummy.h:12: class
PowerProfilerHelperDummy : public PowerProfilerHelper {
Also move into helper.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_profiler_helper_factory.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_helper_factory.h:14: class
PowerProfilerHelperFactory {
ditto

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_profiler_service.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_service.h:30: void
AddObserver(PowerProfilerHost* observer);
AddObserver(PowerProfilerObserver*)?

https://codereview.chromium.org/106223002/diff/1/content/common/devtools_mess...
File content/common/devtools_messages.h (right):

https://codereview.chromium.org/106223002/diff/1/content/common/devtools_mess...
content/common/devtools_messages.h:141:
IPC_MESSAGE_ROUTED0(DevToolsHostMsg_PowerProfileSupported)
Should be a part of protocol.

https://codereview.chromium.org/106223002/diff/1/content/public/browser/brows...
File content/public/browser/browser_thread.h (right):

https://codereview.chromium.org/106223002/diff/1/content/public/browser/brows...
content/public/browser/browser_thread.h:84: POWER_PROFILER,
You don't want a named thread for this.

[pfeldman, 2014-01-17 09:24:13]

@qsr: I did not look at browser_profiler closely and I think it will take time
to polish the API. Feel free to chime in with the review comments here.

[qsr, 2014-01-17 11:50:13]

Preliminary review on the service. Didn't look at the usage at all yet. Before
continuing, could you rework on the API, in particular:

- The threading issue
- The creation of the ``Helper''

https://codereview.chromium.org/106223002/diff/1/content/browser/browser_thre...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/browser_thre...
content/browser/browser_thread_impl.cc:32: "Chrome_PowerProfilerThread", //
POWER_PROFILER
On 2014/01/09 11:38:58, pfeldman wrote:
> Why do you want a named thread for power profiler? Could we get away with an
ad
> hock one?

 You seem to jave defined the thread for the power service. Instead you should
do the following:

1) Have the public API of the power service be asynchronous
2) Inside the power service, use a sequenced worker pool to delegate all of you
work, sending back the result asynchronously. You can use
PostTaskAndReplyWithResult to handle all of this.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/Intel/power_profiler_helper_ia.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/Intel/power_profiler_helper_ia.h:16: #include
<windows.h>
 why is this conditional to windows? Right now, this is windows only, so you
could have only have .h/.cc file with the win implementation.

 You will be able to refactor later on when you have a posix implementation.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/Intel/power_profiler_helper_ia_win.cc
(right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/Intel/power_profiler_helper_ia_win.cc:13: int
PowerProfilerHelperIA::GetData(PowerEvent* data, double* energies,
How is this working? You do not have any information on the size of |data|.
(count could be it, but you are comparing it to ID_COUNT). It works well with
this implementation because you only ever write a single PowerEvent, but your
signature seems to imply you might want to write more than one.

You should instead either change the signature to be explicit about only sending
one data, or, change the signature to take a:

std::array<PowerEvent>* data

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/devtools_power_profiler_host.cc (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/devtools_power_profiler_host.cc:74:
reference_monotonic_time_ = base::TimeTicks::Now();
Those 2 variable are not needed.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/devtools_power_profiler_host.cc:78: double
DevToolsPowerProfilerHost::convertMonotonicTimeToWallTime(
Return a base::Time() there, and compute it with:


base::Time::UnixEpoch() + (tick - base::TimeTicks::UnixEpoch())

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_event.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_event.h:14: enum Type {
Also, pease comment on this variable, at least for the first one. Moreover, am I
missing something but I do not see you ever getting any value for the GPU
counter (in that case, you should remove it from there until you can compute
something). As for the CPU, I'm under the impression you compute a value during
the initialization phase of your Helper implementation, but never after that,
which makes it again not useful at this point.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_profiler_helper.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_helper.h:15: class
PowerProfilerHelper : public base::RefCounted<PowerProfilerHelper>{
PowerDataProvider could be a better choice.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_profiler_host.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_host.h:13: #define
ENABLE_POWER_PROFILER 1
 Using pre-processor variable at this level seems brittle. Moreover, I do not
think the condition is the right one. Right now there is only one implementation
of the power profiler, but we want a generic API that will work on more
platform.

 As you do have a factory to create the service, you should have a boolean on
the service itself telling you if the power profiler is effective or not, and
use it instead of this compilation directive.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_host.h:33: void UnRegister();
Can you add comments on all those methods. Moreover, use Unregister instead.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_host.h:35: Resolution
GetResolution() const { return resolution_; }
Inlined getter should be resolution() { return resolution_; }

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_host.h:41: Resolution resolution_;
Why is this protected? You are setting this in the constructor of the sub class,
but you should be either using the setter, or have a parameter, and put this in
the private section.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_profiler_service.cc (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_service.cc:21: profiler_helper_ = 
PowerProfilerHelperFactory::Create();
You should move the initialize to the creation. That would allow the factory to
try multiple implementation of the Helper (as Pavel said, name should be changed
to something more descriptive). After that, the helper should have a boolean
that tells you if it is a dummy or not, and you will be able to replace your
ENABLE_POWER_PROFILER preprocessor directive by this.

 That would allow to use this API with multiple architectures.

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_service.cc:117: double enegries[3]
= {0.0};
Is this 3 PowerEvent::ID_COUNT?

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_profiler_service.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_service.h:48: void
AccumulateEnergies(double* energies, int count);
All of those methods await an array of the same size, instead of using ptr +
count, you can defined near your PowerEvent::Type:

typedef int PowerEventArray[ID_COUNT];

and then, use:

void AccumulateEnergies(PowerEventArray* energies)

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_service.h:50:
scoped_ptr<base::OneShotTimer<PowerProfilerService> > query_power_timer_;
Why is that a scoped_ptr, instead of an instance variable?

https://codereview.chromium.org/106223002/diff/1/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/106223002/diff/1/content/content_browser.gypi...
content/content_browser.gypi:1278:
'../third_party/power_gadget/IntelPowerGadgetLib.cpp',
We do not compile source from third-party directly in the content file. You
should create a gyp file in the third-party library and depend on the library.

https://codereview.chromium.org/106223002/diff/1/third_party/power_gadget/REA...
File third_party/power_gadget/README.chromium (right):

https://codereview.chromium.org/106223002/diff/1/third_party/power_gadget/REA...
third_party/power_gadget/README.chromium:5: License: Intel Sample Code License
Did you look at http://www.chromium.org/developers/adding-3rd-party-libraries?

 In particular the part about Chrome Eng Review. You will need a review from 
open-source-third-party-reviews@google.com and security@chromium.org. And we do
not have any third party with this licenses yet, so you should start that early.

https://codereview.chromium.org/106223002/diff/1/third_party/power_gadget/REA...
third_party/power_gadget/README.chromium:8: Intel® Power Gadget library aims to
access real-time data about a 2nd generation or later Intel® Core processor
regarding current estimated processor power, thermal design power (TDP), current
processor frequency, base frequency, current temperature, maximum temperature,
timestamps and elapsed time. The data is obtained from the Model Specific
Registers (MSRs) that are available only in 2nd Generation Intel® Core or later
processors.
Can you limit the text to 80 columns?

[qsr, 2014-01-17 11:51:07]

Also, start the conversation with the open-source-third-party-reviews@google.com
to make sure we can include the intel code.

[Pan, 2014-01-18 04:30:36]

thanks qsr and pfeldman,

power reading API in energy library(Intel energy library) is sync and immediate
return. the reason I used a independent thread is to assure the profiling
interval is good (as you see the timer). so is it suitable to use sequenced
worker pool?

thanks
Pan

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
File content/browser/power_profiler/power_profiler_service.h (right):

https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
content/browser/power_profiler/power_profiler_service.h:38: Status status();
I'm thinking, Public API of PowerProfilerService should be:

void AddObserver(PowerProfilerObserver*)
void RemoveObserver(PowerProfilerObserver*)
void UpdateResolution();

static PowerProfilerService* GetInstance(); // singleton usage
Status status(); // service status.

Snapshot() and GetEnergy(PowerEvent::Type) is designed for Power Monitor API I
proposed to measure energy in JavaScript, won't be used here, so they can be
removed for the time being.

In this way, PowerProfilerObservers are async notified when timer queried power
data, is it ok?

[tonyg, 2014-01-18 15:48:57]

Just a quick question, I haven't really reviewed anything yet, is this going to
work without admin?

[Pan, 2014-01-20 01:39:18]

On 2014/01/18 15:48:57, tonyg wrote:
> Just a quick question, I haven't really reviewed anything yet, is this going
to
> work without admin?

Intel energy library should be installed with admin(out of chrome), while it is
one time effort.
After that, it works w/o admin.

thanks
Pan

[qsr, 2014-01-20 09:16:47]

On 2014/01/18 04:30:36, Pan wrote:
> thanks qsr and pfeldman,
> 
> power reading API in energy library(Intel energy library) is sync and
immediate
> return. the reason I used a independent thread is to assure the profiling
> interval is good (as you see the timer). so is it suitable to use sequenced
> worker pool?

 Yes. You do not want a named thread for this. Now, you have two option, either
using the generic sequenced worker pool, or define your own task runner in the
service. Until proven otherwise, I think the first option is the right one. I do
not think this is critical enough to warrant its own thread.

> thanks
> Pan
> 
>
https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
> File content/browser/power_profiler/power_profiler_service.h (right):
> 
>
https://codereview.chromium.org/106223002/diff/1/content/browser/power_profil...
> content/browser/power_profiler/power_profiler_service.h:38: Status status();
> I'm thinking, Public API of PowerProfilerService should be:
> 
> void AddObserver(PowerProfilerObserver*)
> void RemoveObserver(PowerProfilerObserver*)
> void UpdateResolution();
> 
> static PowerProfilerService* GetInstance(); // singleton usage
> Status status(); // service status.
> 
> Snapshot() and GetEnergy(PowerEvent::Type) is designed for Power Monitor API I
> proposed to measure energy in JavaScript, won't be used here, so they can be
> removed for the time being.
> 
> In this way, PowerProfilerObservers are async notified when timer queried
power
> data, is it ok?

[qsr, 2014-01-20 09:16:54]

https://chromiumcodereview.appspot.com/106223002/diff/1/content/browser/power...
File content/browser/power_profiler/power_profiler_service.h (right):

https://chromiumcodereview.appspot.com/106223002/diff/1/content/browser/power...
content/browser/power_profiler/power_profiler_service.h:38: Status status();
On 2014/01/18 04:30:37, Pan wrote:
> I'm thinking, Public API of PowerProfilerService should be:
> 
> void AddObserver(PowerProfilerObserver*)
> void RemoveObserver(PowerProfilerObserver*)
> void UpdateResolution();
> 
> static PowerProfilerService* GetInstance(); // singleton usage
> Status status(); // service status.
> 
> Snapshot() and GetEnergy(PowerEvent::Type) is designed for Power Monitor API I
> proposed to measure energy in JavaScript, won't be used here, so they can be
> removed for the time being.
> 
> In this way, PowerProfilerObservers are async notified when timer queried
power
> data, is it ok?

 Mostly yes. The only one I would ask about is status. Can you get this
information synchronously if the service is mostly delegating everything to
another thread? If yes, then the API is fine, otherwise, the method should take
a callback.

 Also, when documenting this class, please specify on which threads those
methods can be called. It will be a lot simpler if it can only be called on the
main thread, otherwise, you will need to remember on which thread the
AddObserver was called to be able to call all the observer method from the same
thread.

[Pan, 2014-01-20 09:43:51]

On 2014/01/20 09:16:47, qsr wrote:
> On 2014/01/18 04:30:36, Pan wrote:
> > thanks qsr and pfeldman,
> > 
> > power reading API in energy library(Intel energy library) is sync and
> immediate
> > return. the reason I used a independent thread is to assure the profiling
> > interval is good (as you see the timer). so is it suitable to use sequenced
> > worker pool?
> 
>  Yes. You do not want a named thread for this. Now, you have two option,
either
> using the generic sequenced worker pool, or define your own task runner in the
> service. Until proven otherwise, I think the first option is the right one. I
do
> not think this is critical enough to warrant its own thread.

Hi qsr, 
Thanks a lot for your help.
A quick question, IIRC, timer should be destructed on the thread that run the
first task, so that in power_profiler_service, can we use the same thread in
SequencedWorkerPool every time for tasks?

Pan

[Pan, 2014-01-20 10:09:29]

On 2014/01/20 09:16:54, qsr wrote:
>
https://chromiumcodereview.appspot.com/106223002/diff/1/content/browser/power...
> File content/browser/power_profiler/power_profiler_service.h (right):
> 
>
https://chromiumcodereview.appspot.com/106223002/diff/1/content/browser/power...
> content/browser/power_profiler/power_profiler_service.h:38: Status status();
> On 2014/01/18 04:30:37, Pan wrote:
> > I'm thinking, Public API of PowerProfilerService should be:
> > 
> > void AddObserver(PowerProfilerObserver*)
> > void RemoveObserver(PowerProfilerObserver*)
> > void UpdateResolution();
> > 
> > static PowerProfilerService* GetInstance(); // singleton usage
> > Status status(); // service status.
> > 
> > Snapshot() and GetEnergy(PowerEvent::Type) is designed for Power Monitor API
I
> > proposed to measure energy in JavaScript, won't be used here, so they can be
> > removed for the time being.
> > 
> > In this way, PowerProfilerObservers are async notified when timer queried
> power
> > data, is it ok?
> 
>  Mostly yes. The only one I would ask about is status. Can you get this
> information synchronously if the service is mostly delegating everything to
> another thread? If yes, then the API is fine, otherwise, the method should
take
> a callback.
> 
>  Also, when documenting this class, please specify on which threads those
> methods can be called. It will be a lot simpler if it can only be called on
the
> main thread, otherwise, you will need to remember on which thread the
> AddObserver was called to be able to call all the observer method from the
same
> thread.

you are right, while, I scanned the place where status() is used in the
observers, and found
observers just only care if service is "UNINITIALIZED" or not. and
"UNINITIALIZED" to "INITIALIZED" only could happens when Service is
constructed(GetInstance), so I think status() can be replaced by bool
Is_Initialized(), the latter one is safe, how do you think of it?

thanks
Pan

[qsr, 2014-01-20 10:16:30]

On 2014/01/20 10:09:29, Pan wrote:
> On 2014/01/20 09:16:54, qsr wrote:
> >
>
https://chromiumcodereview.appspot.com/106223002/diff/1/content/browser/power...
> > File content/browser/power_profiler/power_profiler_service.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/106223002/diff/1/content/browser/power...
> > content/browser/power_profiler/power_profiler_service.h:38: Status status();
> > On 2014/01/18 04:30:37, Pan wrote:
> > > I'm thinking, Public API of PowerProfilerService should be:
> > > 
> > > void AddObserver(PowerProfilerObserver*)
> > > void RemoveObserver(PowerProfilerObserver*)
> > > void UpdateResolution();
> > > 
> > > static PowerProfilerService* GetInstance(); // singleton usage
> > > Status status(); // service status.
> > > 
> > > Snapshot() and GetEnergy(PowerEvent::Type) is designed for Power Monitor
API
> I
> > > proposed to measure energy in JavaScript, won't be used here, so they can
be
> > > removed for the time being.
> > > 
> > > In this way, PowerProfilerObservers are async notified when timer queried
> > power
> > > data, is it ok?
> > 
> >  Mostly yes. The only one I would ask about is status. Can you get this
> > information synchronously if the service is mostly delegating everything to
> > another thread? If yes, then the API is fine, otherwise, the method should
> take
> > a callback.
> > 
> >  Also, when documenting this class, please specify on which threads those
> > methods can be called. It will be a lot simpler if it can only be called on
> the
> > main thread, otherwise, you will need to remember on which thread the
> > AddObserver was called to be able to call all the observer method from the
> same
> > thread.
> 
> you are right, while, I scanned the place where status() is used in the
> observers, and found
> observers just only care if service is "UNINITIALIZED" or not. and
> "UNINITIALIZED" to "INITIALIZED" only could happens when Service is
> constructed(GetInstance), so I think status() can be replaced by bool
> Is_Initialized(), the latter one is safe, how do you think of it?

 The method seem fine, but the name is probably not the right one. See my
comment about not using compile time information to decide if power profiling is
available or not. This method is probably the IsAvailable() method I was
speaking about, and it should delegate the call to the ``PowerProfilerHelper''
(renamed). So initialization should be an internal state, while the fact that
there is a profiler available should be in the public API.  

> 
> thanks
> Pan

[qsr, 2014-01-20 10:23:47]

On 2014/01/20 09:43:51, Pan wrote:
> On 2014/01/20 09:16:47, qsr wrote:
> > On 2014/01/18 04:30:36, Pan wrote:
> > > thanks qsr and pfeldman,
> > > 
> > > power reading API in energy library(Intel energy library) is sync and
> > immediate
> > > return. the reason I used a independent thread is to assure the profiling
> > > interval is good (as you see the timer). so is it suitable to use
sequenced
> > > worker pool?
> > 
> >  Yes. You do not want a named thread for this. Now, you have two option,
> either
> > using the generic sequenced worker pool, or define your own task runner in
the
> > service. Until proven otherwise, I think the first option is the right one.
I
> do
> > not think this is critical enough to warrant its own thread.
> 
> Hi qsr, 
> Thanks a lot for your help.
> A quick question, IIRC, timer should be destructed on the thread that run the
> first task, so that in power_profiler_service, can we use the same thread in
> SequencedWorkerPool every time for tasks?

 You are right about destruction of the timer. In fact all methods of the timer
out of the constructor should always be called from the same thread. It means
that you cannot make the timer live on the generic worker pool. You can still
use your own worker pool if you limit it to a single thread. Now, the easier
way, and the one I would advise unless you have a strong reason not to is to
have the timer live on the main thread, but the method it calls will then
delegate to the generic worker pool, something like that:

timer_.Start(FROM_HERE, base::TimeDelta::FromFoo(...), base::Bind(&OnTimer,
this))

void PowerProfilerService::OnTimer() {
  base::PostTaskAndReplyWithResult(
    BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(token_)
    FROM_HERE,
    base::Bind(...),
    base::Bind(...));
}

> 
> Pan

[Pan, 2014-01-22 14:07:11]

Hi qsr, pfeldman and tonyg,

thanks for your help, I think I addressed most of the comments, except
DevToolsPowerProfilerHost send power event to DevTools front-end is not
implemented.
please have another round of review, as the license issue is not quite clear
right now, I'm going to split this CL into several small one, something like:
1, service + provider( dummy provider) + observer
2, DevToolsPowerProfilerHost
3, provider on IA and third_party
if you don't mind, I will address your comments in the small CLs tomorrow. 
thanks a lot!

Pan

[qsr, 2014-01-22 15:30:00]

Looked at the service/observer/provider part.

https://codereview.chromium.org/106223002/diff/400001/content/browser/power_p...
File content/browser/power_profiler/power_profiler_service.cc (right):

https://codereview.chromium.org/106223002/diff/400001/content/browser/power_p...
content/browser/power_profiler/power_profiler_service.cc:25:
DCHECK(base::MessageLoop::current());
Not really useful. More useful would be to test you are on the UI thread.

https://codereview.chromium.org/106223002/diff/400001/content/browser/power_p...
content/browser/power_profiler/power_profiler_service.cc:30:
query_power_timer_.Stop();
Not needed. Destroying the timer will stop it.

https://codereview.chromium.org/106223002/diff/400001/content/browser/power_p...
content/browser/power_profiler/power_profiler_service.cc:75:
query_power_timer_.Stop();
Stop is not useful, restarting it will stop the current one.

https://codereview.chromium.org/106223002/diff/400001/content/browser/power_p...
content/browser/power_profiler/power_profiler_service.cc:111: void
PowerProfilerService::Notify(PowerEvent* event) {
You do not want to notify from your task runner, as the observer have absolutely
no reason to be thread safe. Delegate the power event back to the main thread.
Moreover, that also creates a concurrency issue accessing observers_.

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
File content/public/browser/power_data_provider.h (right):

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_data_provider.h:14: // A class used to monitor the
power usage and tell profiler the power data
The comment seems not to apply to this object. This is just a data point, it
doesn't do anything.

Moreover, comments must be sentences and so end with a period.

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_data_provider.h:29: // A class used to GET power
usage
No need for uppercase.

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_data_provider.h:34: // Return true when success
Can you be more precise and what this method those? Does it return a single
power event? Does it needs to be initialized beforehand?

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_data_provider.h:37: virtual bool IsDummy() const {
return false; }
You might not nee a dummy provider in fact, and so won't need such a method: the
only user of the provider is the service, so the factory can simply return NULL
if there is no provider, and the service can check for this.

 If you still prefer to use a dummy implementation, this method should be
renamed to something like: IsAvailable()

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_data_provider.h:40: PowerDataProvider() { }
I'm not sure you want to protect this. When writing test, having the factory
being the only one able to create a provider will be cumbersome.

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_data_provider.h:68: static PowerDataProvider*
Create();
You can return a scoped_ptr<PowerDataProvider> here, documenting the ownership.

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
File content/public/browser/power_profiler_observer.h (right):

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_profiler_observer.h:16: class CONTENT_EXPORT
PowerProfilerObserver
I'm not sure to understand why the object should be
base::RefCountedThreadSafe<PowerProfilerObserver>

My guess is that you need this for your current implementation, and because this
is not a pure virtual base class, you cannot have multiple inheritance.

I would instead propose that this class becomes a pure virtual base class, so:

Remove Register and Unregister, the calls to AddObserver/RemoveObserver are
usual, and you do not need to have them here.

You can also remove the SetResolution, and instead add this as a parameter of
AddObserver. Another solution is to have the Resolution be a property of the
Service instead of one of the observer. It is simpler, and I do not understand
why you would need that at the observer level.

After that, you only have a single virtual method here.

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_profiler_observer.h:41: virtual void
Send(PowerEvent*) { }
Make this a pure virtual method, as I do not see why you would implement this
and not override this method. Also you can probably change it to
OnPowerEvent(const PowerEvent&)

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
File content/public/browser/power_profiler_service.h (right):

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_profiler_service.h:30: bool IsAvailable() { return
status_ !=  UNINITIALIZED; }
This is not a simple getter, so should not be inlined.

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_profiler_service.h:57: size_t delay_; //
milliseconds
Use directly a base::TimeTicks

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_profiler_service.h:58: size_t
delays_[PowerProfilerObserver::TYPE_COUNT]; // kinds of delays
Same here.

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_profiler_service.h:60:
std::set<PowerProfilerObserver* > observers_;
Use base/observer_list.h

[Pan, 2014-01-23 09:37:42]

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
File content/public/browser/power_profiler_observer.h (right):

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_profiler_observer.h:16: class CONTENT_EXPORT
PowerProfilerObserver
On 2014/01/22 15:30:01, qsr wrote:
> I'm not sure to understand why the object should be
> base::RefCountedThreadSafe<PowerProfilerObserver>
> 
> My guess is that you need this for your current implementation, and because
this
> is not a pure virtual base class, you cannot have multiple inheritance.
> 
it is there because previously, observer called in UI thread while register
itself in PowerProfiler thread. now we can remove it.

> I would instead propose that this class becomes a pure virtual base class, so:
> 
> Remove Register and Unregister, the calls to AddObserver/RemoveObserver are
> usual, and you do not need to have them here.
> 
> You can also remove the SetResolution, and instead add this as a parameter of
> AddObserver. Another solution is to have the Resolution be a property of the
> Service instead of one of the observer. It is simpler, and I do not understand
> why you would need that at the observer level.

I'm thinking there may be kinds of observers that have different resolution
requirement, like HIGH for profile point(in DevTools), LOW for measurement
point(measure in a period). if resolution_ lives in a observer, we can not only
adjust profling QoS to HIGH when HIGH customer comes, but also back to LOW when
it leaves. 
Register and Unregister, I don't have strong preference to keep or remove them,
how do you think of it?

thanks 
Pan

[qsr, 2014-01-23 10:08:04]

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
File content/public/browser/power_profiler_observer.h (right):

https://codereview.chromium.org/106223002/diff/400001/content/public/browser/...
content/public/browser/power_profiler_observer.h:16: class CONTENT_EXPORT
PowerProfilerObserver
On 2014/01/23 09:37:43, Pan wrote:
> On 2014/01/22 15:30:01, qsr wrote:
> > I'm not sure to understand why the object should be
> > base::RefCountedThreadSafe<PowerProfilerObserver>
> > 
> > My guess is that you need this for your current implementation, and because
> this
> > is not a pure virtual base class, you cannot have multiple inheritance.
> > 
> it is there because previously, observer called in UI thread while register
> itself in PowerProfiler thread. now we can remove it.
> 
> > I would instead propose that this class becomes a pure virtual base class,
so:
> > 
> > Remove Register and Unregister, the calls to AddObserver/RemoveObserver are
> > usual, and you do not need to have them here.
> > 
> > You can also remove the SetResolution, and instead add this as a parameter
of
> > AddObserver. Another solution is to have the Resolution be a property of the
> > Service instead of one of the observer. It is simpler, and I do not
understand
> > why you would need that at the observer level.
> 
> I'm thinking there may be kinds of observers that have different resolution
> requirement, like HIGH for profile point(in DevTools), LOW for measurement
> point(measure in a period). if resolution_ lives in a observer, we can not
only
> adjust profling QoS to HIGH when HIGH customer comes, but also back to LOW
when
> it leaves. 

 Then you should have a virtual method returning the resolution, and let
implementation decide what to do: either return a constant value, or have a
setter. You can also have a default implementation (in a .cc file) if you think
it makes sense.

> Register and Unregister, I don't have strong preference to keep or remove
them,
> how do you think of it?

 Then they should be removed.

> 
> thanks 
> Pan

[pfeldman, 2014-02-25 16:30:45]

Is this out of date?

[Pan, 2014-02-25 23:43:40]

On 2014/02/25 16:30:45, pfeldman wrote:
> Is this out of date?

yes, this one is out of date, it's spitted into several patches.
one in review https://codereview.chromium.org/140583003/.

thanks
Pan

[Pan, 2014-02-26 00:24:37]

On 2014/02/25 23:43:40, Pan wrote:
> On 2014/02/25 16:30:45, pfeldman wrote:
> > Is this out of date?
> 
> yes, this one is out of date, it's spitted into several patches.
> one in review https://codereview.chromium.org/140583003/.
> 
> thanks
> Pan

this CL is used to show the overview of power profiler prototype, code for
landing is split into several one.
Sorry to introduce confusion. I will close this issue.