Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2954413004:
[GRC] Tab-level CPU Attribution (Closed)
Description[GRC] Tab-level CPU Attribution
This CL enabled CPU usage attribution at a tab-level granularity within GRC.
Platform CPU usage metrics can only perform measurements at a process-level granularity.
To attribute the measured process-level CPU usage measurements to tabs, the following calculation is performed as follows:
1. A render process's CPU usage is equally attributable to all of it's children tabs (i.e. process_cpu_usage / num_tabs).
2. For a tab that uses multiple render processes (i.e. out-of-process iFrames), it's CPU usage is the sum of (1) for each of these render processes.
R=lpy@chromium.org,zhenw@chromium.org
TBR=oysteine@chromium.org
BUG=691886
Review-Url: https://codereview.chromium.org/2954413004
Cr-Commit-Position: refs/heads/master@{#483900}
Committed: https://chromium.googlesource.com/chromium/src/+/3668e4d71c7cefe86d1edf8a4e01d1bbaeb5d674
Patch Set 1 #Patch Set 2 : Add RecalculateProperty to CUs #Patch Set 3 : Add unittest #
Total comments: 16
Patch Set 4 : Cleanup #
Total comments: 2
Patch Set 5 : Address reviewer feedback #Patch Set 6 : Address reviewer feedback #Patch Set 7 : Add frame coordination unit impl #
Total comments: 20
Patch Set 8 : Add unittests #Patch Set 9 : Address reviewer feedback #
Total comments: 34
Patch Set 10 : Address reviewer feedback #Patch Set 11 : Update unittest comments #
Total comments: 20
Patch Set 12 : Address reviewer feedback #Patch Set 13 : Rebase #Dependent Patchsets: Messages
Total messages: 47 (21 generated)
Description was changed from ========== Initialize CL BUG= ========== to ========== NOCOMMIT Process CPU Usage Plumbing BUG= ==========
matthalp@google.com changed reviewers: + lpy@chromium.org
Description was changed from ========== NOCOMMIT Process CPU Usage Plumbing BUG= ========== to ========== [GRC] Tab-level CPU Attribution TODO: - Mock ProcessCoordinationUnitImpl for unittest - Add some comments for unittest - Add comment for calculation algorithm - Add bug # R=lpy@chromium.org,zhenw@chromium.org TBR=oysteine@chromium.org BUG= ==========
matthalp@google.com changed reviewers: + oysteine@chromium.org, zhenw@chromium.org
matthalp@google.com changed required reviewers: + lpy@chromium.org, zhenw@chromium.org
lpy, zhenw PTAL: I have some small enhancements to make still, but it is good enough for a preliminary review
lpy@chromium.org changed required reviewers: - lpy@chromium.org, zhenw@chromium.org
Thanks for putting up the change, I left some comments, ptal. https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc (right): https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:109: process_resource_coordinator->AddChild(*tab_resource_coordinator_); should we add it above when tab_resource_coordinator_ is created? https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:48: const CoordinationUnitImpl* coordination_unit); I think we have id() that returns the CoordinationUnitID, right? So maybe we don't need methods above. https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc:39: IsWebContentsCoordinationUnit); Just a lambda here?
https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc (right): https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:109: process_resource_coordinator->AddChild(*tab_resource_coordinator_); On 2017/06/28 at 07:22:53, lpy wrote: > should we add it above when tab_resource_coordinator_ is created? Yes, that's fine. https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:48: const CoordinationUnitImpl* coordination_unit); On 2017/06/28 at 07:22:53, lpy wrote: > I think we have id() that returns the CoordinationUnitID, right? So maybe we don't need methods above. A pattern of checking coordination unit types has emerged, so I figured having these helper functions might make sense. What does everyone else think? https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc:39: IsWebContentsCoordinationUnit); On 2017/06/28 at 07:22:53, lpy wrote: > Just a lambda here? Yes, if we decide to remove the helper methods.
By the way, Nat just added me as an owner of resource_coordinator folder this morning. So we should be able to land the CLs. Just still TBR oystein in case he wants to double check. https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc (right): https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:109: process_resource_coordinator->AddChild(*tab_resource_coordinator_); On 2017/06/28 14:38:12, matthalp wrote: > On 2017/06/28 at 07:22:53, lpy wrote: > > should we add it above when tab_resource_coordinator_ is created? > > Yes, that's fine. The relationship between process and tab is tricky. In OOPIF world, it is hard to say which is parent or child. Instead of direct parent/child link, I am leaning toward having a helper function to find the associated tabs in a process. Similarly, we can have another helper function to find the processes for a tab. https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_factory.cc (right): https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_factory.cc:28: case CoordinationUnitType::kWebContents: The |tab_resource_coordinator_| on the browser side uses kWebContents already. Does it mean the |tab_resource_coordinator_| was created as CUImpl before (the default case)? Any reason of doing that before? https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:48: const CoordinationUnitImpl* coordination_unit); On 2017/06/28 14:38:12, matthalp wrote: > On 2017/06/28 at 07:22:53, lpy wrote: > > I think we have id() that returns the CoordinationUnitID, right? So maybe we > don't need methods above. > > A pattern of checking coordination unit types has emerged, so I figured having > these helper functions might make sense. What does everyone else think? It is probably not needed for now. https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:75: virtual void RecalculateProperty(mojom::PropertyType property) {} It may be better to move this upper, e.g., after the overridden section. If you want to keep it with this block of property related functions, you can move the whole block up. https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h:29: double CalculateCPUUsage(); Should this be private?
> https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... > File > chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc > (right): > > https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... > chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:109: > process_resource_coordinator->AddChild(*tab_resource_coordinator_); > On 2017/06/28 14:38:12, matthalp wrote: > > On 2017/06/28 at 07:22:53, lpy wrote: > > > should we add it above when tab_resource_coordinator_ is created? > > > > Yes, that's fine. > > The relationship between process and tab is tricky. In OOPIF world, it is hard > to say which is parent or child. Instead of direct parent/child link, I am > leaning toward having a helper function to find the associated tabs in a > process. Similarly, we can have another helper function to find the processes > for a tab. I think we can introduce a constraint here. Basically we want to define that a ProcessCU is the parent of a TabCU if there is at least a frame owned by ProcessCU that is also owned by TabCU. And this will then become a concept inside GRC, and should be documented explicitly inside the codebase(have a README inside services/resource_coordinator), but the documentation is not the purpose of this patch.
On 2017/06/28 17:24:21, lpy wrote: > > > https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... > > File > > > chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc > > (right): > > > > > https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... > > > chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:109: > > process_resource_coordinator->AddChild(*tab_resource_coordinator_); > > On 2017/06/28 14:38:12, matthalp wrote: > > > On 2017/06/28 at 07:22:53, lpy wrote: > > > > should we add it above when tab_resource_coordinator_ is created? > > > > > > Yes, that's fine. > > > > The relationship between process and tab is tricky. In OOPIF world, it is hard > > to say which is parent or child. Instead of direct parent/child link, I am > > leaning toward having a helper function to find the associated tabs in a > > process. Similarly, we can have another helper function to find the processes > > for a tab. > > I think we can introduce a constraint here. Basically we want to define that a > ProcessCU is the parent of a TabCU if there is at least a frame owned by > ProcessCU that is also owned by TabCU. And this will then become a concept > inside GRC, and should be documented explicitly inside the codebase(have a > README inside services/resource_coordinator), but the documentation is not the > purpose of this patch. I see the parent/child links in the CU graph as well defined/perceived concepts in Chrome. The relationship between tab and process is not well defined. It's true that we could define the relationship in GRC. But using a helper function can avoid the possible misalignment and is not much more complex and will not add much overhead.
Description was changed from ========== [GRC] Tab-level CPU Attribution TODO: - Mock ProcessCoordinationUnitImpl for unittest - Add some comments for unittest - Add comment for calculation algorithm - Add bug # R=lpy@chromium.org,zhenw@chromium.org TBR=oysteine@chromium.org BUG= ========== to ========== [GRC] Tab-level CPU Attribution This CL enabled CPU usage attribution at a tab-level granularity within GRC. Platform CPU usage metrics can only perform measurements at a process-level granularity. To attribute the measured process-level CPU usage measurements to tabs, the following calculation is performed as follows: 1. A render process's CPU usage is equally attributable to all of it's children tabs (i.e. process_cpu_usage / num_tabs). 2. For a tab that uses multiple render processes (i.e. out-of-process iFrames), it's CPU usage is the sum of (1) for each of these render processes. R=lpy@chromium.org,zhenw@chromium.org TBR=oysteine@chromium.org BUG= ==========
Description was changed from ========== [GRC] Tab-level CPU Attribution This CL enabled CPU usage attribution at a tab-level granularity within GRC. Platform CPU usage metrics can only perform measurements at a process-level granularity. To attribute the measured process-level CPU usage measurements to tabs, the following calculation is performed as follows: 1. A render process's CPU usage is equally attributable to all of it's children tabs (i.e. process_cpu_usage / num_tabs). 2. For a tab that uses multiple render processes (i.e. out-of-process iFrames), it's CPU usage is the sum of (1) for each of these render processes. R=lpy@chromium.org,zhenw@chromium.org TBR=oysteine@chromium.org BUG= ========== to ========== [GRC] Tab-level CPU Attribution This CL enabled CPU usage attribution at a tab-level granularity within GRC. Platform CPU usage metrics can only perform measurements at a process-level granularity. To attribute the measured process-level CPU usage measurements to tabs, the following calculation is performed as follows: 1. A render process's CPU usage is equally attributable to all of it's children tabs (i.e. process_cpu_usage / num_tabs). 2. For a tab that uses multiple render processes (i.e. out-of-process iFrames), it's CPU usage is the sum of (1) for each of these render processes. R=lpy@chromium.org,zhenw@chromium.org TBR=oysteine@chromium.org BUG=691886 ==========
Can you also reply the comments in the CL, so we know which ones have been addressed? It seems to me some of them are not. https://codereview.chromium.org/2954413004/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:72: } As we discussed yesterday, this should be like: auto tabs = GetAssociatedTabCUs(); for (auto tab : tabs) tab->RecalculateProperty(...); And then GetAssociatedTabCUs() worry about whether using traversal or parent-child link. And I see you removed the check of "IsWebContentsCoordinationUnit(child)". Why is that removed in this version? The helper function can probably cover that though.
All feedback has been responded to and addressed in the code. PTAL https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc (right): https://codereview.chromium.org/2954413004/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:109: process_resource_coordinator->AddChild(*tab_resource_coordinator_); On 2017/06/28 at 17:15:15, Zhen Wang wrote: > On 2017/06/28 14:38:12, matthalp wrote: > > On 2017/06/28 at 07:22:53, lpy wrote: > > > should we add it above when tab_resource_coordinator_ is created? > > > > Yes, that's fine. > > The relationship between process and tab is tricky. In OOPIF world, it is hard to say which is parent or child. Instead of direct parent/child link, I am leaning toward having a helper function to find the associated tabs in a process. Similarly, we can have another helper function to find the processes for a tab. Done. https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_factory.cc (right): https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_factory.cc:28: case CoordinationUnitType::kWebContents: On 2017/06/28 at 17:15:15, Zhen Wang wrote: > The |tab_resource_coordinator_| on the browser side uses kWebContents already. Does it mean the |tab_resource_coordinator_| was created as CUImpl before (the default case)? Any reason of doing that before? Now that we have actually WebContents-specific functionality for CoordinationUnits I have created a WebContentCoordinationUnitImpl class and that is now what is called here. https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:48: const CoordinationUnitImpl* coordination_unit); On 2017/06/28 at 17:15:15, Zhen Wang wrote: > On 2017/06/28 14:38:12, matthalp wrote: > > On 2017/06/28 at 07:22:53, lpy wrote: > > > I think we have id() that returns the CoordinationUnitID, right? So maybe we > > don't need methods above. > > > > A pattern of checking coordination unit types has emerged, so I figured having > > these helper functions might make sense. What does everyone else think? > > It is probably not needed for now. I still have not removed these helper methods, but can remove them if you still feel they should go away. https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:75: virtual void RecalculateProperty(mojom::PropertyType property) {} On 2017/06/28 at 17:15:15, Zhen Wang wrote: > It may be better to move this upper, e.g., after the overridden section. If you want to keep it with this block of property related functions, you can move the whole block up. Done. https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/40001/services/resource_coord... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h:29: double CalculateCPUUsage(); On 2017/06/28 at 17:15:15, Zhen Wang wrote: > Should this be private? Done. https://codereview.chromium.org/2954413004/diff/60001/services/resource_coord... File services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/60001/services/resource_coord... services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:72: } On 2017/06/29 at 20:26:22, Zhen Wang wrote: > As we discussed yesterday, this should be like: > > auto tabs = GetAssociatedTabCUs(); > for (auto tab : tabs) > tab->RecalculateProperty(...); > > And then GetAssociatedTabCUs() worry about whether using traversal or parent-child link. Done. > And I see you removed the check of "IsWebContentsCoordinationUnit(child)". Why is that removed in this version? The helper function can probably cover that though. Yes, it's been moved into GetAssociatedWebContentsCoordinationUnits()
Thanks for cleaning this up! It's getting into shape. I have some more comments. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:64: GetAssociatedWebContentsCoordinationUnits(); Comments needed for those two. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:71: void SetProperty(mojom::PropertyType property, base::Value value); I lean toward not having comments for ClearProperty, GetProperty, and SetProperty. Their function names are clear enough. The above virtual function names are also clear. But I think it is recommended to add comments to them to guide people they need to override them. Especially, those two will call NOTREACHED here. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:73: virtual void RecalculateProperty(mojom::PropertyType property) {} nit: Move virtual functions to earlier positions and put virtual functions together. The style I prefer is (notice the blank lines): // Utility functions ... // comments virtual ... GetAssociatedProcessCoordinationUnits(); // comments virtual ... GetAssociatedWebContentsCoordinationUnits(); //comments virtual void RecalculateProperty(mojom::PropertyType property) {} // Overridden from mojom::CoordinationUnit: void SendEvent void GetID ... others ... base::Value GetProperty(mojom::PropertyType property) const; void SetProperty(mojom::PropertyType property, base::Value value); void ClearProperty(mojom::PropertyType property); https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h:23: class FrameCoordinationUnitImpl : public CoordinationUnitImpl { Be aware that lpy has another CL to add this class (for different purpose): https://codereview.chromium.org/2967563002/ You two can coordinate on landing this. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:90: for (auto* tab : GetAssociatedWebContentsCoordinationUnits()) { Add a TODO here to describe that you will move the propagation part out of this function. So set-property-and-propagate is a general supported way. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc:37: Can we also add a test for GetAssociatedWebContentsCoordinationUnits() for process CU? It is not trivial with traversal. And the test can help if we later switch implementation. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc:58: : 0.0; nit: I feel the following is more intuitive. Either is fine, I do not have strong opinion here. double process_cpu_usage = process_cpu_usage_value.IsType(base::Value::Type::NONE) ? 0.0 : process_cpu_usage_value.GetDouble(); https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc:59: cpu_usage += process_cpu_usage / static_cast<double>(tabs_in_process); Do we need this explicit cast? https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h:31: std::set<CoordinationUnitImpl*> GetAssociatedProcessCoordinationUnits() Similar to the one in process CU, having a test for this helps. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h:32: override; nit: should this "override" fit in the previous line?
> https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... > File > services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h > (right): > > https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... > services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h:23: > class FrameCoordinationUnitImpl : public CoordinationUnitImpl { > Be aware that lpy has another CL to add this class (for different purpose): > https://codereview.chromium.org/2967563002/ > > You two can coordinate on landing this. not sure why frame cu is needed here, since it seems to me it's not doing anything. This patch is doing tab level cpu attribution, and the "walk" function we discussed is to serve this purpose, IMHO anything that is not serving this purpose should be in follow-up patch with functionality.
Feedback has been addressed. PTAL https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:64: GetAssociatedWebContentsCoordinationUnits(); On 2017/06/30 at 16:27:00, Zhen Wang wrote: > Comments needed for those two. Done -- let me know if this is clear or too descriptive. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:71: void SetProperty(mojom::PropertyType property, base::Value value); On 2017/06/30 at 16:27:00, Zhen Wang wrote: > I lean toward not having comments for ClearProperty, GetProperty, and SetProperty. Their function names are clear enough. > > The above virtual function names are also clear. But I think it is recommended to add comments to them to guide people they need to override them. Especially, those two will call NOTREACHED here. Done. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:73: virtual void RecalculateProperty(mojom::PropertyType property) {} On 2017/06/30 at 16:27:00, Zhen Wang wrote: > nit: Move virtual functions to earlier positions and put virtual functions together. > > The style I prefer is (notice the blank lines): > > // Utility functions > ... > > // comments > virtual ... GetAssociatedProcessCoordinationUnits(); > > // comments > virtual ... GetAssociatedWebContentsCoordinationUnits(); > > //comments > virtual void RecalculateProperty(mojom::PropertyType property) {} > > // Overridden from mojom::CoordinationUnit: > void SendEvent > void GetID > ... others ... > > base::Value GetProperty(mojom::PropertyType property) const; > void SetProperty(mojom::PropertyType property, base::Value value); > void ClearProperty(mojom::PropertyType property); I agree this is more clear -- done. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h:23: class FrameCoordinationUnitImpl : public CoordinationUnitImpl { On 2017/06/30 at 16:27:01, Zhen Wang wrote: > Be aware that lpy has another CL to add this class (for different purpose): > https://codereview.chromium.org/2967563002/ > > You two can coordinate on landing this. Sounds good. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/process_coordination_unit_impl.cc:90: for (auto* tab : GetAssociatedWebContentsCoordinationUnits()) { On 2017/06/30 at 16:27:01, Zhen Wang wrote: > Add a TODO here to describe that you will move the propagation part out of this function. So set-property-and-propagate is a general supported way. Done. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/process_coordination_unit_impl_unittest.cc:37: On 2017/06/30 at 16:27:01, Zhen Wang wrote: > Can we also add a test for GetAssociatedWebContentsCoordinationUnits() for process CU? It is not trivial with traversal. And the test can help if we later switch implementation. Done. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc:58: : 0.0; On 2017/06/30 at 16:27:01, Zhen Wang wrote: > nit: I feel the following is more intuitive. Either is fine, I do not have strong opinion here. > > double process_cpu_usage = > process_cpu_usage_value.IsType(base::Value::Type::NONE) > ? 0.0 > : process_cpu_usage_value.GetDouble(); Done. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc:59: cpu_usage += process_cpu_usage / static_cast<double>(tabs_in_process); On 2017/06/30 at 16:27:01, Zhen Wang wrote: > Do we need this explicit cast? We don't -- done. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h:31: std::set<CoordinationUnitImpl*> GetAssociatedProcessCoordinationUnits() On 2017/06/30 at 16:27:01, Zhen Wang wrote: > Similar to the one in process CU, having a test for this helps. I've created a unittest in coordination_unit_impl_unittests.* that tests this functionality. Let me know what you think and we can decide whether or not to break it apart into separate files. https://codereview.chromium.org/2954413004/diff/120001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h:32: override; On 2017/06/30 at 16:27:01, Zhen Wang wrote: > nit: should this "override" fit in the previous line? Unfortunately that would force the line to be over 80 lines. The code format tucks "override" underneath.
https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... File services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. not sure why frame cu is needed here, since it seems to me it's not doing anything. This patch is doing tab level cpu attribution, and the "walk" function we discussed is to serve this purpose, IMHO anything that is not serving this purpose should be in follow-up patch with functionality.
https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... File services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/06/30 at 19:17:27, lpy wrote: > not sure why frame cu is needed here, since it seems to me it's not doing > anything. > > This patch is doing tab level cpu attribution, and the "walk" function we > discussed is to serve this purpose, IMHO anything that is not serving this > purpose should be in follow-up patch with functionality. The FrameCoordinationUnitImpl is here because it does play a role in performing the walk. It overrides GetAssociated*CoordinationUnits() in doing so. FrameCoordinationUnitImpl::GetAssociated*CoordinationUnits() is called inside ProcessCoordinationUnitImpl::GetAssociatedWebContentsCoordinationUnits() and WebContentsCoordinationUnitImpl::GetAssociatedProcessCoordinationUnits().
Thanks for adding the tests! https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:273: child->GetChildCoordinationUnitsOfType(type)) { Why do we need grandchildren? I guess the hidden question is: Does a subframe's parent include WebContentsCU? I think the answer is yes? https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:292: parent->GetParentCoordinationUnitsOfType(type)) { Why do we need grand parents? https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:58: GetAssociatedWebContentsCoordinationUnits(); I thought about this more. I think it is better to just have one interface: // Get associated CUs of Type for this CU. virtual std::set<CUImpl*> GetAssociatedCUs(Type); Here is the reason. Having an interface here means it applies to all child classes. But not all child classes will want to get associated process or web contents CU. Using one generic interface will be better, so child class can customize for itself. And with this, you do not need to mention NOTREACHED in the comment, which is internal detail. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:75: virtual void RecalculateProperty(mojom::PropertyType property) {} nit: move this above with other virtual functions. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc (right): https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:145: CoordinationUnitID child_cu_id(CoordinationUnitType::kFrame, std::string()); Those 2 lines are not needed any more. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:214: CoordinationUnitID cu_id(CoordinationUnitType::kWebContents, std::string()); ditto https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:255: tabs_associated_with_process.find(tab_coordination_unit.get())); Using set::count is more readable, e.g., EXPECT_EQ(1, tabs_associated_with_process.count(tab_cu)); https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:262: processes_associated_with_tab.find(process_coordination_unit.get())); ditto https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:287: tabs_associated_with_process.find(tab_coordination_unit.get())); ditto https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:290: tabs_associated_with_process.find(other_tab_coordination_unit.get())); ditto https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:297: processes_associated_with_tab.find(process_coordination_unit.get())); ditto https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:303: processes_associated_with_tab.find(process_coordination_unit.get())); ditto https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:330: tabs_associated_with_process.find(tab_coordination_unit.get())); ditto https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:337: tabs_associated_with_other_process.find(tab_coordination_unit.get())); ditto https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:344: processes_associated_with_tab.find(process_coordination_unit.get())); ditto https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:347: other_process_coordination_unit.get())); ditto
On 2017/06/30 19:22:24, matthalp wrote: > https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... > File > services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h > (right): > > https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... > services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h:1: > // Copyright 2017 The Chromium Authors. All rights reserved. > On 2017/06/30 at 19:17:27, lpy wrote: > > not sure why frame cu is needed here, since it seems to me it's not doing > > anything. > > > > This patch is doing tab level cpu attribution, and the "walk" function we > > discussed is to serve this purpose, IMHO anything that is not serving this > > purpose should be in follow-up patch with functionality. > > The FrameCoordinationUnitImpl is here because it does play a role in performing > the walk. It overrides GetAssociated*CoordinationUnits() in doing so. > FrameCoordinationUnitImpl::GetAssociated*CoordinationUnits() is called inside > ProcessCoordinationUnitImpl::GetAssociatedWebContentsCoordinationUnits() and > WebContentsCoordinationUnitImpl::GetAssociatedProcessCoordinationUnits(). ah, my mistake that I didn't see it. Thanks for pointing this out.
PTAL https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:273: child->GetChildCoordinationUnitsOfType(type)) { On 2017/06/30 at 19:54:07, Zhen Wang wrote: > Why do we need grandchildren? > > I guess the hidden question is: > Does a subframe's parent include WebContentsCU? I think the answer is yes? Currently, a CU cannot become a parent if it's already one of your ancestors: https://cs.chromium.org/chromium/src/services/resource_coordinator/coordinati... Due to this fact, the topology of a simple OOPIF example would be: P T \ / F P \ / F Another way to think of this is that when a a child frame get's added it is first attached to it's parent frame. Then when then the subframe is attempted to be added as a child of the process it will fail because the parent frame is already a child of it. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:292: parent->GetParentCoordinationUnitsOfType(type)) { On 2017/06/30 at 19:54:07, Zhen Wang wrote: > Why do we need grand parents? See above response. Happy to set up a call to discuss further, but this is how GRC has worked since the code landed. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:58: GetAssociatedWebContentsCoordinationUnits(); On 2017/06/30 at 19:54:07, Zhen Wang wrote: > I thought about this more. I think it is better to just have one interface: > > // Get associated CUs of Type for this CU. > virtual std::set<CUImpl*> GetAssociatedCUs(Type); > > Here is the reason. Having an interface here means it applies to all child classes. But not all child classes will want to get associated process or web contents CU. Using one generic interface will be better, so child class can customize for itself. > > And with this, you do not need to mention NOTREACHED in the comment, which is internal detail. Sounds good. With the caveat that you are not associated with yourself if you match the CoordinationUnitType. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:75: virtual void RecalculateProperty(mojom::PropertyType property) {} On 2017/06/30 at 19:54:07, Zhen Wang wrote: > nit: move this above with other virtual functions. Done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc (right): https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:145: CoordinationUnitID child_cu_id(CoordinationUnitType::kFrame, std::string()); On 2017/06/30 at 19:54:08, Zhen Wang wrote: > Those 2 lines are not needed any more. Thank you for catching this -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:214: CoordinationUnitID cu_id(CoordinationUnitType::kWebContents, std::string()); On 2017/06/30 at 19:54:08, Zhen Wang wrote: > ditto Thank you for catching this -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:255: tabs_associated_with_process.find(tab_coordination_unit.get())); On 2017/06/30 at 19:54:08, Zhen Wang wrote: > Using set::count is more readable, e.g., > EXPECT_EQ(1, tabs_associated_with_process.count(tab_cu)); I like this better too -- thank you -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:262: processes_associated_with_tab.find(process_coordination_unit.get())); On 2017/06/30 at 19:54:08, Zhen Wang wrote: > ditto I like this better too -- thank you -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:287: tabs_associated_with_process.find(tab_coordination_unit.get())); On 2017/06/30 at 19:54:08, Zhen Wang wrote: > ditto I like this better too -- thank you -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:290: tabs_associated_with_process.find(other_tab_coordination_unit.get())); On 2017/06/30 at 19:54:08, Zhen Wang wrote: > ditto I like this better too -- thank you -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:297: processes_associated_with_tab.find(process_coordination_unit.get())); On 2017/06/30 at 19:54:07, Zhen Wang wrote: > ditto I like this better too -- thank you -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:303: processes_associated_with_tab.find(process_coordination_unit.get())); On 2017/06/30 at 19:54:07, Zhen Wang wrote: > ditto I like this better too -- thank you -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:330: tabs_associated_with_process.find(tab_coordination_unit.get())); On 2017/06/30 at 19:54:08, Zhen Wang wrote: > ditto I like this better too -- thank you -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:337: tabs_associated_with_other_process.find(tab_coordination_unit.get())); On 2017/06/30 at 19:54:08, Zhen Wang wrote: > ditto I like this better too -- thank you -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:344: processes_associated_with_tab.find(process_coordination_unit.get())); On 2017/06/30 at 19:54:08, Zhen Wang wrote: > ditto I like this better too -- thank you -- done. https://codereview.chromium.org/2954413004/diff/160001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:347: other_process_coordination_unit.get())); On 2017/06/30 at 19:54:07, Zhen Wang wrote: > ditto I like this better too -- thank you -- done. :)
lgtm from my side :) please wait for Zhen's feedback.
lpy@chromium.org changed reviewers: + estark@chromium.org
+estark@ for security review on mojom. estark@, ptal.
mojom lgtm
LGTM with nits https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:303: return std::set<CoordinationUnitImpl*>(); I think we still need NOTREACHED() here to prevent this being called? https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:60: // by invoking their their |RecalculateProperty|. nit: 1. duplicated "their" 2. not need to mention "by invoking ...". That is internal details. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:61: virtual void PropagateProperty(mojom::PropertyType property) {} This is not used any where in the CL? Just remove it for now. Add later when using it. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:344: // OP: other_process_coordination_unit Thanks for the comments here! https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h:21: enum class CoordinationUnitType : uint8_t; Why is this needed? Can't we just include coordination_unit_types.h? https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h:22: enum class CoordinationUnitType : uint8_t; Why is this needed? Can't we just include coordination_unit_types.h? https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h:25: enum class CoordinationUnitType : uint8_t; Why is this needed? Can't we just include coordination_unit_types.h? https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc:28: case CoordinationUnitType::kProcess: { nit: I guess we do not need this "{" (and "}" on line 45)? https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h:24: enum class CoordinationUnitType : uint8_t; Why is this needed? Can't we just include coordination_unit_types.h?
LGTM with nits https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:303: return std::set<CoordinationUnitImpl*>(); I think we still need NOTREACHED() here to prevent this being called? https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:60: // by invoking their their |RecalculateProperty|. nit: 1. duplicated "their" 2. not need to mention "by invoking ...". That is internal details. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:61: virtual void PropagateProperty(mojom::PropertyType property) {} This is not used any where in the CL? Just remove it for now. Add later when using it. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:344: // OP: other_process_coordination_unit Thanks for the comments here! https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h:21: enum class CoordinationUnitType : uint8_t; Why is this needed? Can't we just include coordination_unit_types.h? https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h:22: enum class CoordinationUnitType : uint8_t; Why is this needed? Can't we just include coordination_unit_types.h? https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h:25: enum class CoordinationUnitType : uint8_t; Why is this needed? Can't we just include coordination_unit_types.h? https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc:28: case CoordinationUnitType::kProcess: { nit: I guess we do not need this "{" (and "}" on line 45)? https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h:24: enum class CoordinationUnitType : uint8_t; Why is this needed? Can't we just include coordination_unit_types.h?
The CQ bit was checked by matthalp@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by matthalp@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by matthalp@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org, lpy@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2954413004/#ps240001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1498880084761300,
"parent_rev": "e608c0815c9a97106ce1d9d9b406e72eab147cd2", "commit_rev":
"3668e4d71c7cefe86d1edf8a4e01d1bbaeb5d674"}
Message was sent while issue was closed.
Description was changed from ========== [GRC] Tab-level CPU Attribution This CL enabled CPU usage attribution at a tab-level granularity within GRC. Platform CPU usage metrics can only perform measurements at a process-level granularity. To attribute the measured process-level CPU usage measurements to tabs, the following calculation is performed as follows: 1. A render process's CPU usage is equally attributable to all of it's children tabs (i.e. process_cpu_usage / num_tabs). 2. For a tab that uses multiple render processes (i.e. out-of-process iFrames), it's CPU usage is the sum of (1) for each of these render processes. R=lpy@chromium.org,zhenw@chromium.org TBR=oysteine@chromium.org BUG=691886 ========== to ========== [GRC] Tab-level CPU Attribution This CL enabled CPU usage attribution at a tab-level granularity within GRC. Platform CPU usage metrics can only perform measurements at a process-level granularity. To attribute the measured process-level CPU usage measurements to tabs, the following calculation is performed as follows: 1. A render process's CPU usage is equally attributable to all of it's children tabs (i.e. process_cpu_usage / num_tabs). 2. For a tab that uses multiple render processes (i.e. out-of-process iFrames), it's CPU usage is the sum of (1) for each of these render processes. R=lpy@chromium.org,zhenw@chromium.org TBR=oysteine@chromium.org BUG=691886 Review-Url: https://codereview.chromium.org/2954413004 Cr-Commit-Position: refs/heads/master@{#483900} Committed: https://chromium.googlesource.com/chromium/src/+/3668e4d71c7cefe86d1edf8a4e01... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/3668e4d71c7cefe86d1edf8a4e01...
Message was sent while issue was closed.
https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:60: // by invoking their their |RecalculateProperty|. On 2017/06/30 23:37:50, Zhen Wang wrote: > nit: > 1. duplicated "their" > 2. not need to mention "by invoking ...". That is internal details. Just a reminder this is not yet fixed. This can be fixed in your follow-up CL using the propagation. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc:28: case CoordinationUnitType::kProcess: { On 2017/06/30 23:37:51, Zhen Wang wrote: > nit: I guess we do not need this "{" (and "}" on line 45)? And this.
Message was sent while issue was closed.
https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.cc:303: return std::set<CoordinationUnitImpl*>(); On 2017/06/30 at 23:37:50, Zhen Wang wrote: > I think we still need NOTREACHED() here to prevent this being called? Done. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:60: // by invoking their their |RecalculateProperty|. On 2017/06/30 at 23:37:50, Zhen Wang wrote: > nit: > 1. duplicated "their" > 2. not need to mention "by invoking ...". That is internal details. Done. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl.h:61: virtual void PropagateProperty(mojom::PropertyType property) {} On 2017/06/30 at 23:37:50, Zhen Wang wrote: > This is not used any where in the CL? Just remove it for now. Add later when using it. Done. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest.cc:344: // OP: other_process_coordination_unit On 2017/06/30 at 23:37:50, Zhen Wang wrote: > Thanks for the comments here! No problem! https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h:21: enum class CoordinationUnitType : uint8_t; On 2017/06/30 at 23:37:50, Zhen Wang wrote: > Why is this needed? Can't we just include coordination_unit_types.h? Done. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h:22: enum class CoordinationUnitType : uint8_t; On 2017/06/30 at 23:37:51, Zhen Wang wrote: > Why is this needed? Can't we just include coordination_unit_types.h? Done. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h:25: enum class CoordinationUnitType : uint8_t; On 2017/06/30 at 23:37:51, Zhen Wang wrote: > Why is this needed? Can't we just include coordination_unit_types.h? Done. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.cc:28: case CoordinationUnitType::kProcess: { On 2017/06/30 at 23:37:51, Zhen Wang wrote: > nit: I guess we do not need this "{" (and "}" on line 45)? The compiler complains if the braces are removed. Let's revisit moving them in a future CL. https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... File services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h (right): https://codereview.chromium.org/2954413004/diff/200001/services/resource_coor... services/resource_coordinator/coordination_unit/web_contents_coordination_unit_impl.h:24: enum class CoordinationUnitType : uint8_t; On 2017/06/30 at 23:37:51, Zhen Wang wrote: > Why is this needed? Can't we just include coordination_unit_types.h? Done. |
