Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
972083002:
Report utility process JS memory in task manager. (Closed)
|
Created:
5 years, 9 months ago by Anand Mistry (off Chromium) Modified:
5 years, 7 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, darin (slow to review), ben+mojo_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@v8-pac-oop Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReport utility process JS memory in task manager.
BUG=467832
Committed: https://crrev.com/faa231a441d9890d8dee349b578dee7abed70501
Cr-Commit-Position: refs/heads/master@{#330670}
Patch Set 1 #Patch Set 2 : Clean up. #
Total comments: 15
Patch Set 3 : Redesign. #
Total comments: 23
Patch Set 4 : Move to //chrome and add test. #Patch Set 5 : Update comment. #
Total comments: 1
Patch Set 6 : Try fixing the build. #Patch Set 7 : Try to fix build... again. #
Total comments: 15
Patch Set 8 : Review changes. #Patch Set 9 : Remove include. #Patch Set 10 : Rebase and fix build. #
Total comments: 2
Patch Set 11 : Add comment. #Messages
Total messages: 44 (13 generated)
amistry@chromium.org changed reviewers: + afakhry@chromium.org, sievers@chromium.org
afakhry: Added you because of the task manager re-write. What do you think? I need to report JS memory usage from a utility process, but _really_ don't want to use Chrome IPC. I'm hoping to extend this to the renderer process and replace the Chrome IPC there with this Mojo interface. For now, just do it for the utility process.
amistry@chromium.org changed reviewers: + sammc@chromium.org
FYI, there's no rush to review this. I'll be gone for a week and a bit, so take your time.
afakhry@chromium.org changed reviewers: + nick@chromium.org
Adding +ncarter as he's the owner of the old task manager.
And to see what this would look like if it was applied to the renderer process, take a look at https://codereview.chromium.org/1081323003/
On 2015/04/14 06:57:20, Anand Mistry wrote: > And to see what this would look like if it was applied to the renderer process, > take a look at https://codereview.chromium.org/1081323003/ Awesome. I just wanted to ACK this CL to let you know it's on my radar. I took a quick pass to get the gist of it already, and will be able to provide proper feedback in the next few days. I may send you a CL that will revamp the one browsertest that calls GetV8Memory -- that should help with this effort too, and make it possible to add a browsertest for child process v8 memory usage. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tas...
Overall I'm excited to get this data into the task manager. Thanks for taking this on. I'm pushing back on the use of Locks -- PostTask and single-threaded objects are the Chrome way, I don't see anything going on here (other than expedience) that would justify deviation from that norm. If there's something subtle I'm missing, please convince me. https://codereview.chromium.org/972083002/diff/20001/chrome/utility/chrome_co... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/972083002/diff/20001/chrome/utility/chrome_co... chrome/utility/chrome_content_utility_client.cc:96: ResourceUsageReporterImpl( explicit https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... File content/browser/process_resource_usage_impl.h (right): https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... content/browser/process_resource_usage_impl.h:17: Could you add a class comment that tries to answers the following questions: - Who is this for - How do I get one - What does it do https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... content/browser/process_resource_usage_impl.h:38: mutable base::Lock lock_; What would this look like if we used PostTask instead of Locks? Locks are considered an anti-pattern in chrome ["We discourage locking and thread-safe objects." http://dev.chromium.org/developers/design-documents/threading ] [ https://groups.google.com/a/chromium.org/d/msg/chromium-dev/dR0XM1OFaeA/6CFFd... ] [ https://groups.google.com/a/chromium.org/d/msg/chromium-dev/dR0XM1OFaeA/Y1zSW... ] https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... content/browser/process_resource_usage_impl.h:43: scoped_refptr<base::MessageLoopProxy> mojo_message_loop_; This is my first time looking at code that uses mojo. I'm actually a little surprised that these messages are handled on the browser process IO thread and written into a struct that is lock-protected -- I thought part of the promise of mojo was to "bury the pipes," which I expected to mean something like: mojo would take care of forwarding the IPC messages where they needed to be consumed, e.g., the UI thread, and it wouldn't be users caller's responsibility to get data to the IO thread? Is it possible/conceivable to make this mojo interface live on the UI thread (where the BrowserChildProcessObservers are?) Seems like that would eliminate the need for the lock, and make it possible to notify observers (somehow) that fresh stats had been collected? https://codereview.chromium.org/972083002/diff/20001/content/public/browser/c... File content/public/browser/child_process_data.h (right): https://codereview.chromium.org/972083002/diff/20001/content/public/browser/c... content/public/browser/child_process_data.h:37: scoped_refptr<ProcessResourceUsage> process_resource_usage; As implemented, this modification does plumb the process_resource_usage everywhere it needs to go, and effortlessly. But is it too much of a shortcut? Does this member belong in this class? This type, ChildProcessData, up until now has functioned as the signature/id of the process: it's passed to every method of BrowserChildProcessObserver (process creations and deletions alike); historically (ages ago) I think it was intended for use as a map key. It's a struct, copyable, and those are usually reserved for passing around plain data. But here you're adding a scoped pointer to a stateful object that has a Refresh method and whose values could change synchronously at any moment. That doesn't feel like something that belongs in a copyable-identifier-struct-like to me. Is there a way to implement this without adding a stateful object to ChildProcessData? How much grosser is that? https://codereview.chromium.org/972083002/diff/20001/content/public/browser/p... File content/public/browser/process_resource_usage.h (right): https://codereview.chromium.org/972083002/diff/20001/content/public/browser/p... content/public/browser/process_resource_usage.h:19: virtual void Refresh() = 0; A question for afakhry: Will you (with the rewrite) be okay with an interface that doesn't provide notification to the task manager when the V8 stats are updated? Because that's what this is doing, and it seems that limitation is somewhat ingrained in the design here, since the model is "ChildProcessHost publishes a ProcessResourceUsage for all listeners to peek at" rather than the listener giving a completion callback. If we want the completion notification it might be good to figure it out now.
https://codereview.chromium.org/972083002/diff/20001/chrome/browser/task_mana... File chrome/browser/task_manager/child_process_resource_provider.cc (right): https://codereview.chromium.org/972083002/diff/20001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:93: resource_usage_(resource_usage) { Can you come up with a TaskManagerBrowserTest for the resource usage reporting? This is a little more work than usual because afaik we don't have much test coverage currently for any of the Child process reporting in the task manager. But it should be straightforward... here's some hints. Assuming it isn't reverted again, TaskManagerBrowserTest.WebWorkerJSHeapMemory ( pending rewrite: WebWorkerhttps://codereview.chromium.org/1090433002/ ) can be a good baseline. The recipe is something like: - If you have one handy, copy code from some non-taskmanager browsertest that creates a child process that would have a v8 instance. - Add a MatchUtilityProcess() or whatever method to task_manager_browsertest_util.cc, assuming there's not one there already. - Force the child process to do something that would cause at least X memory to be consumed (ideally X is significantly larger than v8's initial memory footprint, though if X needs to be zero, that is probably decent enough). - Wait for the task manager rows. For bonus points you can also: [1] Hide and then show the task manager, and assert that the child process remains. This is valuable because the "loop over existing child processes" and "listen for new child processes" codepaths are different enough to be worth exercising both. [2] test the KillProcess codepath (make sure the task manager row disappears when a child process dies). Let me know if you want help -- I'll happily write the test for you if you can point me at example code to force creation of the appropriate child/utility process type.
https://codereview.chromium.org/972083002/diff/20001/content/public/browser/p... File content/public/browser/process_resource_usage.h (right): https://codereview.chromium.org/972083002/diff/20001/content/public/browser/p... content/public/browser/process_resource_usage.h:19: virtual void Refresh() = 0; On 2015/05/01 21:16:52, ncarter wrote: > A question for afakhry: > > Will you (with the rewrite) be okay with an interface that doesn't provide > notification to the task manager when the V8 stats are updated? > > Because that's what this is doing, and it seems that limitation is somewhat > ingrained in the design here, since the model is "ChildProcessHost publishes a > ProcessResourceUsage for all listeners to peek at" rather than the listener > giving a completion callback. > > If we want the completion notification it might be good to figure it out now. I prefer to have the (ChildProcess/Renderer)Tasks listen to V8 and WebCache stats calculation events rather than having to trigger them on each refresh. If there's anything that we can keep constant until it actually changes, then that would be better. There's no point in doing an expensive calculation on each refresh of the task manager if the values don't change.
No code changes to review, but responding to a couple of comments. https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... File content/browser/process_resource_usage_impl.h (right): https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... content/browser/process_resource_usage_impl.h:43: scoped_refptr<base::MessageLoopProxy> mojo_message_loop_; On 2015/05/01 21:16:52, ncarter wrote: > This is my first time looking at code that uses mojo. I'm actually a little > surprised that these messages are handled on the browser process IO thread and > written into a struct that is lock-protected -- I thought part of the promise of > mojo was to "bury the pipes," which I expected to mean something like: mojo > would take care of forwarding the IPC messages where they needed to be consumed, > e.g., the UI thread, and it wouldn't be users caller's responsibility to get > data to the IO thread? Mojo hides all the IPC details, but it still exposes it's threading model. That threading model is the reason for this design. No, you don't need to be on the IO thread (or UI). But you do need to everything on the *same* thread, regardless of that that thread is. This this case, the Mojo connection is created on the IO thread for the utility process, so all the Mojo actions (IPC calls, connection destruction, etc) have to happen on that same thread. It is possible to move a Mojo connection to another thread. I'll look into how complex it would be to use that. > > Is it possible/conceivable to make this mojo interface live on the UI thread > (where the BrowserChildProcessObservers are?) Seems like that would eliminate > the need for the lock, and make it possible to notify observers (somehow) that > fresh stats had been collected? https://codereview.chromium.org/972083002/diff/20001/content/public/browser/p... File content/public/browser/process_resource_usage.h (right): https://codereview.chromium.org/972083002/diff/20001/content/public/browser/p... content/public/browser/process_resource_usage.h:19: virtual void Refresh() = 0; On 2015/05/04 20:30:03, afakhry wrote: > On 2015/05/01 21:16:52, ncarter wrote: > > A question for afakhry: > > > > Will you (with the rewrite) be okay with an interface that doesn't provide > > notification to the task manager when the V8 stats are updated? > > > > Because that's what this is doing, and it seems that limitation is somewhat > > ingrained in the design here, since the model is "ChildProcessHost publishes a > > ProcessResourceUsage for all listeners to peek at" rather than the listener > > giving a completion callback. > > > > If we want the completion notification it might be good to figure it out now. It's not here because it's not necessary for this specific change. However, adding a callback is trivial and necessary for replacing the renderer process V8 usage with this Mojo interface. Take a look at https://codereview.chromium.org/1081323003/ for details. This still requires requesting an update instead of passively listening, but it's already the case that you need to request these stats be updated. This design makes that more explicit. > > I prefer to have the (ChildProcess/Renderer)Tasks listen to V8 and WebCache > stats calculation events rather than having to trigger them on each refresh. If > there's anything that we can keep constant until it actually changes, then that > would be better. There's no point in doing an expensive calculation on each > refresh of the task manager if the values don't change. The current chrome-IPC approach is to poll for these values, and this change won't change that. For something like V8 memory usage, it's a value that you can't observe for changes (and would be expensive to add) so you need to poll anyway.
I haven't written any tests yet. I'd like to get your feedback on this implementation before I do so. https://codereview.chromium.org/972083002/diff/20001/chrome/utility/chrome_co... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/972083002/diff/20001/chrome/utility/chrome_co... chrome/utility/chrome_content_utility_client.cc:96: ResourceUsageReporterImpl( On 2015/05/01 21:16:52, ncarter wrote: > explicit Done. https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... File content/browser/process_resource_usage_impl.h (right): https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... content/browser/process_resource_usage_impl.h:17: On 2015/05/01 21:16:52, ncarter wrote: > Could you add a class comment that tries to answers the following questions: > - Who is this for > - How do I get one > - What does it do Expanded the class comment for the public interface, which I think is more appropriate than adding one here since this is just the implementation. https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... content/browser/process_resource_usage_impl.h:38: mutable base::Lock lock_; On 2015/05/01 21:16:52, ncarter wrote: > What would this look like if we used PostTask instead of Locks? Locks are > considered an anti-pattern in chrome ["We discourage locking and thread-safe > objects." http://dev.chromium.org/developers/design-documents/threading ] [ > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/dR0XM1OFaeA/6CFFd... > ] [ > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/dR0XM1OFaeA/Y1zSW... > ] Done. It's cleaner, but requires some passing around since the Mojo connection must be created on the IO thread. https://codereview.chromium.org/972083002/diff/20001/content/public/browser/c... File content/public/browser/child_process_data.h (right): https://codereview.chromium.org/972083002/diff/20001/content/public/browser/c... content/public/browser/child_process_data.h:37: scoped_refptr<ProcessResourceUsage> process_resource_usage; On 2015/05/01 21:16:52, ncarter wrote: > As implemented, this modification does plumb the process_resource_usage > everywhere it needs to go, and effortlessly. But is it too much of a shortcut? > Does this member belong in this class? > > This type, ChildProcessData, up until now has functioned as the signature/id of > the process: it's passed to every method of BrowserChildProcessObserver (process > creations and deletions alike); historically (ages ago) I think it was intended > for use as a map key. It's a struct, copyable, and those are usually reserved > for passing around plain data. > > But here you're adding a scoped pointer to a stateful object that has a Refresh > method and whose values could change synchronously at any moment. That doesn't > feel like something that belongs in a copyable-identifier-struct-like to me. > > Is there a way to implement this without adding a stateful object to > ChildProcessData? How much grosser is that? Done. It requires more plumbing, but isn't as bad as I thought it would be.
https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... File chrome/browser/task_manager/child_process_resource_provider.cc (right): https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:92: void ChildProcessResource::GetProcessUsageOnIOThread( What do you think of moving the functionality of: - ChildProcessResource::GetProcessUsageOnIOThread(), - ChildProcessResource::OnGetProcessUsageDone() to the factory of the ProcessResourceUsage? Somehow I feel like this code doesn't belong here, but rather the factory should take care of this complexity and the threading model. I think for the ChildProcessResource, all it cares about is maybe something like this: resource_usage_ = content::ProcessResourceUsage::Create(unique_process_id_); and then the ProcessResourceUsage objects tracks whether it's ready or not. That's just my opinion. Nick has the final say in this. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:262: resource_usage_->Refresh(); Is it possible if you add a callback of type: void(size_t, size_t) to ProcessResourceUsage::Refresh()? You Run() this callback when the refresh is complete which will in turn set two member variables inside ChildProcessResource. Those two member variables will be used in ChildProcessResource::GetV8MemoryAllocated() and ChildProcessResource::GetV8MemoryUsed() instead of querying the resource_usage_ every time. What do you think?
This is really coming together nicely. https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... File content/browser/process_resource_usage_impl.h (right): https://codereview.chromium.org/972083002/diff/20001/content/browser/process_... content/browser/process_resource_usage_impl.h:17: On 2015/05/06 02:44:18, Anand Mistry wrote: > On 2015/05/01 21:16:52, ncarter wrote: > > Could you add a class comment that tries to answers the following questions: > > - Who is this for > > - How do I get one > > - What does it do > > Expanded the class comment for the public interface, which I think is more > appropriate than adding one here since this is just the implementation. Makes sense, thank you. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... File chrome/browser/task_manager/child_process_resource_provider.cc (right): https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:92: void ChildProcessResource::GetProcessUsageOnIOThread( On 2015/05/06 17:16:27, afakhry wrote: > What do you think of moving the functionality of: > > - ChildProcessResource::GetProcessUsageOnIOThread(), > - ChildProcessResource::OnGetProcessUsageDone() > > to the factory of the ProcessResourceUsage? > > Somehow I feel like this code doesn't belong here, but rather the factory should > take care of this complexity and the threading model. > I think for the ChildProcessResource, all it cares about is maybe something like > this: > > resource_usage_ = content::ProcessResourceUsage::Create(unique_process_id_); > > and then the ProcessResourceUsage objects tracks whether it's ready or not. > > That's just my opinion. Nick has the final say in this. GetProcessUsageInfo probably can move into ProcessResourceUsage or similar; especially if you switch to using PostTaskAndReply [see next comment...] Some callback point like OnGetProcessUsageDone needs to live in ChildProcessResource; while I see afakhry's point that the InterfacePtrInfo-to-ProcessResourceUsage conversion might be better if hidden in the ProcessResourceUsage class, we risk losing the elegance of WeakPtr-cancellation if the reply callback isn't a member of ChildProcessResource. Here's the best solution I came up with: - use PostTaskAndReplyWithResult - Move the static GetProcessUsageInfo() function into ProcessResourceInfo - Leave OnGetProcessUsageDone as a method of ChildProcessResource. - Change ProcessResourceUsage::Create() so that it accepts a mojo::InterfacePtrInfo instead of a content::ResourceUsageReporterPtr (thereby moving a few lines of code from OnGetProcessUsageDone to ProcessResourceUsage::Create) https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:133: BrowserThread::PostTask( It might work well to structure this as a PostTaskAndReplyWithResult, channging the return type of GetProcessUsageOnIOThread from void to mojo::InterfacePtrInfo<content::ResourceUsageReporter> or a scoped_ptr thereof. I see two advantages to that: one, the IO thread doesn't get to see the WeakPtr at all (so it's easier to comprehend the threading model), and two, the purpose of GetProcessUsageOnIOThread becomes better reflected in type signature. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:144: mojo::InterfacePtrInfo<content::ResourceUsageReporter> info) { Just curious -- do we expect that this approach will work fine if we try create two ProcessResourceUsages for the same process (I think this is less of an issue with utility/child processes, but will certainly come up in the context of renderer processes, where it's common to have two task manager rows reporting stats for the same process. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:262: resource_usage_->Refresh(); On 2015/05/06 17:16:27, afakhry wrote: > Is it possible if you add a callback of type: > void(size_t, size_t) to ProcessResourceUsage::Refresh()? You Run() this callback > when the refresh is complete which will in turn set two member variables inside > ChildProcessResource. Those two member variables will be used in > ChildProcessResource::GetV8MemoryAllocated() and > ChildProcessResource::GetV8MemoryUsed() instead of querying the resource_usage_ > every time. What do you think? amistry has addressed this to my satisfaction in another comment: the existing implementaiton is polling-based and he's only seeking parity with that. afakhry has a good suggestion here and we're all on the same page that it's a good idea, but it will be best to implement it as a follow-on change; this CL already has plenty going on. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:266: if (resource_usage_) It's not clear if a |resource_usage_| will be successfully created for every child process, or just the ones that actually have spun up a V8 instance. Will e.g. the GPU process continue to show "N/A" in the Javascript Memory column, or will it show start showing "0KB"? https://codereview.chromium.org/972083002/diff/40001/content/public/browser/b... File content/public/browser/browser_child_process_host.h (right): https://codereview.chromium.org/972083002/diff/40001/content/public/browser/b... content/public/browser/browser_child_process_host.h:44: static BrowserChildProcessHost* Get(int id); Name this FromID to match the convention established by these existing classes: https://code.google.com/p/chromium/codesearch#search/&q=static.*FromID%5C(.*i... Also I suggest naming the argument process_id or child_process_id -- our convention here is less rigid, seems that there's just one namespace encompassing the various child process ids and render process ids, but they are called different things in different places. I think some of the variation in naming in places is due to worries about pid/process_id confusion since so there are so many int ids that could be meant. https://codereview.chromium.org/972083002/diff/40001/content/public/browser/p... File content/public/browser/process_resource_usage.h (right): https://codereview.chromium.org/972083002/diff/40001/content/public/browser/p... content/public/browser/process_resource_usage.h:14: // Provides resource usage information about a child process. Thanks for adding these comments, they are great. https://codereview.chromium.org/972083002/diff/40001/content/public/browser/p... content/public/browser/process_resource_usage.h:29: // 3. Pass the service to Create(). If you do change the way that the setup dance works in response to the comments in task_manager, make sure you revisit this comment too. https://codereview.chromium.org/972083002/diff/40001/content/public/browser/p... content/public/browser/process_resource_usage.h:49: }; During my the first pass, I missed the pretty important detail of this being in content/ I hope that it'll be acceptable to move the V8 stats gathering from the chrome layer into the content layer, but I don't have the best grasp on the rules about where the boundary is drawn. I expect the content/ OWNERS review to ask for justification for this (in particular, why it couldn't or shouldn't stay up in the chrome/ layer, since it was kept there before) It's probably worth looping jam@ in on this, and you may want to proactively flesh out an explanation in the CL comment. https://codereview.chromium.org/972083002/diff/40001/content/public/common/re... File content/public/common/resource_usage_reporter.mojom (right): https://codereview.chromium.org/972083002/diff/40001/content/public/common/re... content/public/common/resource_usage_reporter.mojom:7: struct ResourceUsageData { Gosh mojo is nice. Feels great to be living in the future.
Moved to //chrome and added a test. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... File chrome/browser/task_manager/child_process_resource_provider.cc (right): https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:92: void ChildProcessResource::GetProcessUsageOnIOThread( On 2015/05/07 20:12:44, ncarter wrote: > On 2015/05/06 17:16:27, afakhry wrote: > > What do you think of moving the functionality of: > > > > - ChildProcessResource::GetProcessUsageOnIOThread(), > > - ChildProcessResource::OnGetProcessUsageDone() > > > > to the factory of the ProcessResourceUsage? > > > > Somehow I feel like this code doesn't belong here, but rather the factory > should > > take care of this complexity and the threading model. > > I think for the ChildProcessResource, all it cares about is maybe something > like > > this: > > > > resource_usage_ = content::ProcessResourceUsage::Create(unique_process_id_); > > > > and then the ProcessResourceUsage objects tracks whether it's ready or not. > > > > That's just my opinion. Nick has the final say in this. > > GetProcessUsageInfo probably can move into ProcessResourceUsage or similar; > especially if you switch to using PostTaskAndReply [see next comment...] > > Some callback point like OnGetProcessUsageDone needs to live in > ChildProcessResource; while I see afakhry's point that the > InterfacePtrInfo-to-ProcessResourceUsage conversion might be better if hidden in > the ProcessResourceUsage class, we risk losing the elegance of > WeakPtr-cancellation if the reply callback isn't a member of > ChildProcessResource. > > Here's the best solution I came up with: > - use PostTaskAndReplyWithResult > - Move the static GetProcessUsageInfo() function into ProcessResourceInfo > - Leave OnGetProcessUsageDone as a method of ChildProcessResource. > - Change ProcessResourceUsage::Create() so that it accepts a > mojo::InterfacePtrInfo instead of a content::ResourceUsageReporterPtr (thereby > moving a few lines of code from OnGetProcessUsageDone to > ProcessResourceUsage::Create) Apart from PostTaskAndReplyWithResult, I'm going to push-back on these suggestions. My reasoning: 1. I don't think the logic for resolving a process id to a Mojo service belongs in ProcessResourceUsage. Right now, that class only knows about the Mojo service and nothing else. The service could be running in a different process, or the same process, it doesn't care. If the constructor was changes to take a process id or ChildProcessData, then it would be different. But... 2. This specific logic only applies to non-renderer processes. There would need to be something different for handling renderer processes (which I plan to do next). As for taking a mojo::InterfacePtrInfo instead of a ResourceUsageReporterPtr, this doesn't really fit into the Mojo way. InterfacePtrInfo should be a rarely used detail, and a connection to the service is canonically represented by a mojo::InterfacePtr<> (which ResourceUsageReporterPtr is a typedef of). In fact, none of the other Mojo services in Chromium use it. That said, there's a better way, but it requires changes that's outside the scope of this CL. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:133: BrowserThread::PostTask( On 2015/05/07 20:12:43, ncarter wrote: > It might work well to structure this as a PostTaskAndReplyWithResult, channging > the return type of GetProcessUsageOnIOThread from void to > mojo::InterfacePtrInfo<content::ResourceUsageReporter> or a scoped_ptr thereof. > > I see two advantages to that: one, the IO thread doesn't get to see the WeakPtr > at all (so it's easier to comprehend the threading model), and two, the purpose > of GetProcessUsageOnIOThread becomes better reflected in type signature. Done. Aside: Many of Mojo's types are move-only, so no need to scoped_ptr<> in many places. Of course, the exception is if you want to avoid a compile-time dependency on the type definition. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:144: mojo::InterfacePtrInfo<content::ResourceUsageReporter> info) { On 2015/05/07 20:12:43, ncarter wrote: > Just curious -- do we expect that this approach will work fine if we try create > two ProcessResourceUsages for the same process (I think this is less of an issue > with utility/child processes, but will certainly come up in the context of > renderer processes, where it's common to have two task manager rows reporting > stats for the same process. Yes. I see no reason why this wouldn't work. Creating two Mojo connections would result in two separate Mojo service implementations being instantiated in the child process. However, as long as those are independent, which they should be, there won't be a problem. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:266: if (resource_usage_) On 2015/05/07 20:12:43, ncarter wrote: > It's not clear if a |resource_usage_| will be successfully created for every > child process, or just the ones that actually have spun up a V8 instance. If the process host has a ServiceRegistry, then yes, it will be created. However, there's no ability to guarantee that the Mojo service is provided by the child process at construction time. So instead, the |resource_usage_| is created and has a default state, which in this case is false for ReportsV8MemoryStats. That'll be changed on the first IPC response from the child process. > > Will e.g. the GPU process continue to show "N/A" in the Javascript Memory > column, or will it show start showing "0KB"? It shows N/A. Basically, everything using this will show N/A until the first IPC response from the child process. https://codereview.chromium.org/972083002/diff/40001/content/public/browser/b... File content/public/browser/browser_child_process_host.h (right): https://codereview.chromium.org/972083002/diff/40001/content/public/browser/b... content/public/browser/browser_child_process_host.h:44: static BrowserChildProcessHost* Get(int id); On 2015/05/07 20:12:44, ncarter wrote: > Name this FromID to match the convention established by these existing classes: > > https://code.google.com/p/chromium/codesearch#search/&q=static.*FromID%5C(.*i... > > Also I suggest naming the argument process_id or child_process_id -- our > convention here is less rigid, seems that there's just one namespace > encompassing the various child process ids and render process ids, but they are > called different things in different places. I think some of the variation in > naming in places is due to worries about pid/process_id confusion since so there > are so many int ids that could be meant. Done. Also clarified the comment to indicate what this ID is. https://codereview.chromium.org/972083002/diff/40001/content/public/browser/p... File content/public/browser/process_resource_usage.h (right): https://codereview.chromium.org/972083002/diff/40001/content/public/browser/p... content/public/browser/process_resource_usage.h:14: // Provides resource usage information about a child process. On 2015/05/07 20:12:44, ncarter wrote: > Thanks for adding these comments, they are great. Acknowledged. https://codereview.chromium.org/972083002/diff/40001/content/public/browser/p... content/public/browser/process_resource_usage.h:49: }; On 2015/05/07 20:12:44, ncarter wrote: > During my the first pass, I missed the pretty important detail of this being in > content/ > > I hope that it'll be acceptable to move the V8 stats gathering from the chrome > layer into the content layer, but I don't have the best grasp on the rules about > where the boundary is drawn. I expect the content/ OWNERS review to ask for > justification for this (in particular, why it couldn't or shouldn't stay up in > the chrome/ layer, since it was kept there before) > > It's probably worth looping jam@ in on this, and you may want to proactively > flesh out an explanation in the CL comment. I've thought about it and this doesn't belong in content, so I've moved it to chrome. It simplifies things a little since there's no longer an interface.
lgtm! https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... File chrome/browser/task_manager/child_process_resource_provider.cc (right): https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:92: void ChildProcessResource::GetProcessUsageOnIOThread( On 2015/05/11 05:41:31, Anand Mistry wrote: > On 2015/05/07 20:12:44, ncarter wrote: > > On 2015/05/06 17:16:27, afakhry wrote: > > > What do you think of moving the functionality of: > > > > > > - ChildProcessResource::GetProcessUsageOnIOThread(), > > > - ChildProcessResource::OnGetProcessUsageDone() > > > > > > to the factory of the ProcessResourceUsage? > > > > > > Somehow I feel like this code doesn't belong here, but rather the factory > > should > > > take care of this complexity and the threading model. > > > I think for the ChildProcessResource, all it cares about is maybe something > > like > > > this: > > > > > > resource_usage_ = content::ProcessResourceUsage::Create(unique_process_id_); > > > > > > and then the ProcessResourceUsage objects tracks whether it's ready or not. > > > > > > That's just my opinion. Nick has the final say in this. > > > > GetProcessUsageInfo probably can move into ProcessResourceUsage or similar; > > especially if you switch to using PostTaskAndReply [see next comment...] > > > > Some callback point like OnGetProcessUsageDone needs to live in > > ChildProcessResource; while I see afakhry's point that the > > InterfacePtrInfo-to-ProcessResourceUsage conversion might be better if hidden > in > > the ProcessResourceUsage class, we risk losing the elegance of > > WeakPtr-cancellation if the reply callback isn't a member of > > ChildProcessResource. > > > > Here's the best solution I came up with: > > - use PostTaskAndReplyWithResult > > - Move the static GetProcessUsageInfo() function into ProcessResourceInfo > > - Leave OnGetProcessUsageDone as a method of ChildProcessResource. > > - Change ProcessResourceUsage::Create() so that it accepts a > > mojo::InterfacePtrInfo instead of a content::ResourceUsageReporterPtr (thereby > > moving a few lines of code from OnGetProcessUsageDone to > > ProcessResourceUsage::Create) > > Apart from PostTaskAndReplyWithResult, I'm going to push-back on these > suggestions. My reasoning: > 1. I don't think the logic for resolving a process id to a Mojo service belongs > in ProcessResourceUsage. Right now, that class only knows about the Mojo service > and nothing else. The service could be running in a different process, or the > same process, it doesn't care. If the constructor was changes to take a process > id or ChildProcessData, then it would be different. But... > 2. This specific logic only applies to non-renderer processes. There would need > to be something different for handling renderer processes (which I plan to do > next). > > As for taking a mojo::InterfacePtrInfo instead of a ResourceUsageReporterPtr, > this doesn't really fit into the Mojo way. InterfacePtrInfo should be a rarely > used detail, and a connection to the service is canonically represented by a > mojo::InterfacePtr<> (which ResourceUsageReporterPtr is a typedef of). In fact, > none of the other Mojo services in Chromium use it. That said, there's a better > way, but it requires changes that's outside the scope of this CL. I'm fine with the compromise you've struck. My initial comments were not informed by any understanding of the difference between InterfacePtrInfo and ResourceUsageReporterPtr; I was more just reasoning about which lines of code would need to be duplicated when we hook this up to WebContentsResourceProvider, and organizing the functions accordingly. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:133: BrowserThread::PostTask( On 2015/05/11 05:41:31, Anand Mistry wrote: > On 2015/05/07 20:12:43, ncarter wrote: > > It might work well to structure this as a PostTaskAndReplyWithResult, > channging > > the return type of GetProcessUsageOnIOThread from void to > > mojo::InterfacePtrInfo<content::ResourceUsageReporter> or a scoped_ptr > thereof. > > > > I see two advantages to that: one, the IO thread doesn't get to see the > WeakPtr > > at all (so it's easier to comprehend the threading model), and two, the > purpose > > of GetProcessUsageOnIOThread becomes better reflected in type signature. > > Done. > > Aside: Many of Mojo's types are move-only, so no need to scoped_ptr<> in many > places. Of course, the exception is if you want to avoid a compile-time > dependency on the type definition. Great to know, thanks! https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:144: mojo::InterfacePtrInfo<content::ResourceUsageReporter> info) { On 2015/05/11 05:41:31, Anand Mistry wrote: > On 2015/05/07 20:12:43, ncarter wrote: > > Just curious -- do we expect that this approach will work fine if we try > create > > two ProcessResourceUsages for the same process (I think this is less of an > issue > > with utility/child processes, but will certainly come up in the context of > > renderer processes, where it's common to have two task manager rows reporting > > stats for the same process. > > Yes. I see no reason why this wouldn't work. Creating two Mojo connections would > result in two separate Mojo service implementations being instantiated in the > child process. However, as long as those are independent, which they should be, > there won't be a problem. Excellent. https://codereview.chromium.org/972083002/diff/40001/chrome/browser/task_mana... chrome/browser/task_manager/child_process_resource_provider.cc:266: if (resource_usage_) On 2015/05/11 05:41:31, Anand Mistry wrote: > On 2015/05/07 20:12:43, ncarter wrote: > > It's not clear if a |resource_usage_| will be successfully created for every > > child process, or just the ones that actually have spun up a V8 instance. > > If the process host has a ServiceRegistry, then yes, it will be created. > However, there's no ability to guarantee that the Mojo service is provided by > the child process at construction time. So instead, the |resource_usage_| is > created and has a default state, which in this case is false for > ReportsV8MemoryStats. That'll be changed on the first IPC response from the > child process. > > > > > Will e.g. the GPU process continue to show "N/A" in the Javascript Memory > > column, or will it show start showing "0KB"? > > It shows N/A. Basically, everything using this will show N/A until the first IPC > response from the child process. Acknowledged. https://codereview.chromium.org/972083002/diff/80001/chrome/browser/task_mana... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/972083002/diff/80001/chrome/browser/task_mana... chrome/browser/task_manager/task_manager_browsertest.cc:872: IDS_UTILITY_PROCESS_PROXY_RESOLVER_NAME)))); Thanks!!!
amistry@chromium.org changed reviewers: + jam@chromium.org
jam@chromium.org: For chrome/content
https://codereview.chromium.org/972083002/diff/120001/chrome/browser/process_... File chrome/browser/process_resource_usage.h (right): https://codereview.chromium.org/972083002/diff/120001/chrome/browser/process_... chrome/browser/process_resource_usage.h:18: // about://memory-internals. is someone tasked to make about:memory-internals to switch to it? perhaps until that's done, this class should live in task_manager directory? https://codereview.chromium.org/972083002/diff/120001/chrome/browser/task_man... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/972083002/diff/120001/chrome/browser/task_man... chrome/browser/task_manager/task_manager_browsertest.cc:126: // Enable out-of-process proxy resolver. Use a trivial PAC script to ensure given that there are other tests that use this base class, it's probably best to only do this for that one test that needs this. you can use a different base class for the new test, and have it subclass this and set a member variable to enable PAC https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... chrome/utility/chrome_content_utility_client.cc:105: size_t total_heap_size = net::ProxyResolverV8::GetTotalHeapSize(); so this looks like a general purpose interface to get JS memory usage. however it only works when the utility process is hosting PAC? can this be queried directly from v8 so that it always works? https://codereview.chromium.org/972083002/diff/120001/content/browser/utility... File content/browser/utility_process_host_impl.cc (right): https://codereview.chromium.org/972083002/diff/120001/content/browser/utility... content/browser/utility_process_host_impl.cc:214: static_cast<ServiceRegistryImpl*>(GetServiceRegistry())->GetWeakPtr()); usually the pattern is that the delegate returns stuff through BrowserChildProcessHostDelegate. so instead of UtilityProcessHostImpl setting something, it should return it in the delegate
nick@chromium.org changed reviewers: + tsepez@chromium.org
+tsepez for security review (hooking up a new mojo interface)
Messages LGTM. https://codereview.chromium.org/972083002/diff/120001/chrome/common/resource_... File chrome/common/resource_usage_reporter.mojom (right): https://codereview.chromium.org/972083002/diff/120001/chrome/common/resource_... chrome/common/resource_usage_reporter.mojom:7: uint64 v8_memory_allocated = 0; nit: I'd call this v8_bytes_allocated and v8_bytes_used to make the units obvious, otherwise what is memory measured in? pages? kilobytes? etc.
https://codereview.chromium.org/972083002/diff/120001/chrome/browser/process_... File chrome/browser/process_resource_usage.h (right): https://codereview.chromium.org/972083002/diff/120001/chrome/browser/process_... chrome/browser/process_resource_usage.h:18: // about://memory-internals. On 2015/05/12 16:12:23, jam wrote: > is someone tasked to make about:memory-internals to switch to it? perhaps until > that's done, this class should live in task_manager directory? Yes, me. I have another CL in progress that switches the renderer to use this and also switches memory-internals. https://codereview.chromium.org/972083002/diff/120001/chrome/browser/task_man... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/972083002/diff/120001/chrome/browser/task_man... chrome/browser/task_manager/task_manager_browsertest.cc:126: // Enable out-of-process proxy resolver. Use a trivial PAC script to ensure On 2015/05/12 16:12:23, jam wrote: > given that there are other tests that use this base class, it's probably best to > only do this for that one test that needs this. > > you can use a different base class for the new test, and have it subclass this > and set a member variable to enable PAC Done. https://codereview.chromium.org/972083002/diff/120001/chrome/common/resource_... File chrome/common/resource_usage_reporter.mojom (right): https://codereview.chromium.org/972083002/diff/120001/chrome/common/resource_... chrome/common/resource_usage_reporter.mojom:7: uint64 v8_memory_allocated = 0; On 2015/05/12 19:18:17, Tom Sepez wrote: > nit: I'd call this v8_bytes_allocated and v8_bytes_used to make the units > obvious, otherwise what is memory measured in? pages? kilobytes? etc. Done. https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... chrome/utility/chrome_content_utility_client.cc:105: size_t total_heap_size = net::ProxyResolverV8::GetTotalHeapSize(); On 2015/05/12 16:12:23, jam wrote: > so this looks like a general purpose interface to get JS memory usage. however > it only works when the utility process is hosting PAC? Yes. > > can this be queried directly from v8 so that it always works? I wanted to, but memory usage is queried from an Isolate, and from what I can tell, there's no way to enumerate all the Isolates in a process. In this case, ProxyResolverV8::Get*HeapSize() queries its internal Isolate. If anything else uses V8, it would need to be queried explicitly. That, or adding the ability to enumerate Isolates to V8. https://codereview.chromium.org/972083002/diff/120001/content/browser/utility... File content/browser/utility_process_host_impl.cc (right): https://codereview.chromium.org/972083002/diff/120001/content/browser/utility... content/browser/utility_process_host_impl.cc:214: static_cast<ServiceRegistryImpl*>(GetServiceRegistry())->GetWeakPtr()); On 2015/05/12 16:12:23, jam wrote: > usually the pattern is that the delegate returns stuff through > BrowserChildProcessHostDelegate. so instead of UtilityProcessHostImpl setting > something, it should return it in the delegate Done.
The CQ bit was checked by amistry@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/972083002/#ps160001 (title: "Remove include.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972083002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by amistry@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/972083002/#ps180001 (title: "Rebase and fix build.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972083002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
argh, sorry for the delay, I missed your message (feel free to ping me if I don't respond in a few hours) https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... chrome/utility/chrome_content_utility_client.cc:105: size_t total_heap_size = net::ProxyResolverV8::GetTotalHeapSize(); On 2015/05/13 00:42:11, Anand Mistry wrote: > On 2015/05/12 16:12:23, jam wrote: > > so this looks like a general purpose interface to get JS memory usage. however > > it only works when the utility process is hosting PAC? > Yes. > > > > > can this be queried directly from v8 so that it always works? > > I wanted to, but memory usage is queried from an Isolate, and from what I can > tell, there's no way to enumerate all the Isolates in a process. In this case, > ProxyResolverV8::Get*HeapSize() queries its internal Isolate. If anything else > uses V8, it would need to be queried explicitly. That, or adding the ability to > enumerate Isolates to V8. hmm, then perhaps this method should be called GetPACJSUsageData... it seem a bit misleading otherwise. have you checked with a v8 expert if there are any other ways?
https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... chrome/utility/chrome_content_utility_client.cc:105: size_t total_heap_size = net::ProxyResolverV8::GetTotalHeapSize(); On 2015/05/15 06:54:22, jam wrote: > On 2015/05/13 00:42:11, Anand Mistry wrote: > > On 2015/05/12 16:12:23, jam wrote: > > > so this looks like a general purpose interface to get JS memory usage. > however > > > it only works when the utility process is hosting PAC? > > Yes. > > > > > > > > can this be queried directly from v8 so that it always works? > > > > I wanted to, but memory usage is queried from an Isolate, and from what I can > > tell, there's no way to enumerate all the Isolates in a process. In this case, > > ProxyResolverV8::Get*HeapSize() queries its internal Isolate. If anything else > > uses V8, it would need to be queried explicitly. That, or adding the ability > to > > enumerate Isolates to V8. > > hmm, then perhaps this method should be called GetPACJSUsageData... it seem a > bit misleading otherwise. Except that https://codereview.chromium.org/1081323003/ uses the same function to present renderer process JS heap usage. Also, my understanding is that the utility process doesn't have any other v8 usage at the moment, and from a user's perspective, there's no change for non-pac utility processes. Of course, that could change and if it does, this function should be extended to support that. > > have you checked with a v8 expert if there are any other ways? No. My plan was to get the PAC use case working so we can launch out-of-process pac. But I'll bring it up with the V8 folks to see if there's a more generic way of doing this. It would be nice since it's really messy in the renderer process.
https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... chrome/utility/chrome_content_utility_client.cc:105: size_t total_heap_size = net::ProxyResolverV8::GetTotalHeapSize(); On 2015/05/15 07:25:41, Anand Mistry wrote: > On 2015/05/15 06:54:22, jam wrote: > > On 2015/05/13 00:42:11, Anand Mistry wrote: > > > On 2015/05/12 16:12:23, jam wrote: > > > > so this looks like a general purpose interface to get JS memory usage. > > however > > > > it only works when the utility process is hosting PAC? > > > Yes. > > > > > > > > > > > can this be queried directly from v8 so that it always works? > > > > > > I wanted to, but memory usage is queried from an Isolate, and from what I > can > > > tell, there's no way to enumerate all the Isolates in a process. In this > case, > > > ProxyResolverV8::Get*HeapSize() queries its internal Isolate. If anything > else > > > uses V8, it would need to be queried explicitly. That, or adding the ability > > to > > > enumerate Isolates to V8. > > > > hmm, then perhaps this method should be called GetPACJSUsageData... it seem a > > bit misleading otherwise. > > Except that https://codereview.chromium.org/1081323003/ uses the same function > to present renderer process JS heap usage. Also, my understanding is that the > utility process doesn't have any other v8 usage at the moment, and from a user's > perspective, there's no change for non-pac utility processes. Of course, that > could change and if it does, this function should be extended to support that. > > > > > have you checked with a v8 expert if there are any other ways? > > No. My plan was to get the PAC use case working so we can launch out-of-process > pac. But I'll bring it up with the V8 folks to see if there's a more generic way > of doing this. It would be nice since it's really messy in the renderer process. I asked the V8 folks about this and the response is no, there's no way to do this. Each Isolate is independent and V8 doesn't know about them. There's no way to enumerate them or get the total memory usage.
lgtm https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... chrome/utility/chrome_content_utility_client.cc:105: size_t total_heap_size = net::ProxyResolverV8::GetTotalHeapSize(); On 2015/05/15 07:25:41, Anand Mistry wrote: > On 2015/05/15 06:54:22, jam wrote: > > On 2015/05/13 00:42:11, Anand Mistry wrote: > > > On 2015/05/12 16:12:23, jam wrote: > > > > so this looks like a general purpose interface to get JS memory usage. > > however > > > > it only works when the utility process is hosting PAC? > > > Yes. > > > > > > > > > > > can this be queried directly from v8 so that it always works? > > > > > > I wanted to, but memory usage is queried from an Isolate, and from what I > can > > > tell, there's no way to enumerate all the Isolates in a process. In this > case, > > > ProxyResolverV8::Get*HeapSize() queries its internal Isolate. If anything > else > > > uses V8, it would need to be queried explicitly. That, or adding the ability > > to > > > enumerate Isolates to V8. > > > > hmm, then perhaps this method should be called GetPACJSUsageData... it seem a > > bit misleading otherwise. > > Except that https://codereview.chromium.org/1081323003/ uses the same function > to present renderer process JS heap usage. My comment is about net::ProxyResolverV8::GetTotalHeapSize > Also, my understanding is that the > utility process doesn't have any other v8 usage at the moment, and from a user's > perspective, there's no change for non-pac utility processes. Of course, that > could change and if it does, this function should be extended to support that. > > > > > have you checked with a v8 expert if there are any other ways? > > No. My plan was to get the PAC use case working so we can launch out-of-process > pac. But I'll bring it up with the V8 folks to see if there's a more generic way > of doing this. It would be nice since it's really messy in the renderer process. https://codereview.chromium.org/972083002/diff/120001/chrome/utility/chrome_c... chrome/utility/chrome_content_utility_client.cc:105: size_t total_heap_size = net::ProxyResolverV8::GetTotalHeapSize(); On 2015/05/19 04:30:50, Anand Mistry wrote: > On 2015/05/15 07:25:41, Anand Mistry wrote: > > On 2015/05/15 06:54:22, jam wrote: > > > On 2015/05/13 00:42:11, Anand Mistry wrote: > > > > On 2015/05/12 16:12:23, jam wrote: > > > > > so this looks like a general purpose interface to get JS memory usage. > > > however > > > > > it only works when the utility process is hosting PAC? > > > > Yes. > > > > > > > > > > > > > > can this be queried directly from v8 so that it always works? > > > > > > > > I wanted to, but memory usage is queried from an Isolate, and from what I > > can > > > > tell, there's no way to enumerate all the Isolates in a process. In this > > case, > > > > ProxyResolverV8::Get*HeapSize() queries its internal Isolate. If anything > > else > > > > uses V8, it would need to be queried explicitly. That, or adding the > ability > > > to > > > > enumerate Isolates to V8. > > > > > > hmm, then perhaps this method should be called GetPACJSUsageData... it seem > a > > > bit misleading otherwise. > > > > Except that https://codereview.chromium.org/1081323003/ uses the same function > > to present renderer process JS heap usage. Also, my understanding is that the > > utility process doesn't have any other v8 usage at the moment, and from a > user's > > perspective, there's no change for non-pac utility processes. Of course, that > > could change and if it does, this function should be extended to support that. > > > > > > > > have you checked with a v8 expert if there are any other ways? > > > > No. My plan was to get the PAC use case working so we can launch > out-of-process > > pac. But I'll bring it up with the V8 folks to see if there's a more generic > way > > of doing this. It would be nice since it's really messy in the renderer > process. > > I asked the V8 folks about this and the response is no, there's no way to do > this. Each Isolate is independent and V8 doesn't know about them. There's no way > to enumerate them or get the total memory usage. I see, ok thanks for checking. I don't want to block the awesome out of process PAC work, so ok for now. https://codereview.chromium.org/972083002/diff/180001/chrome/common/resource_... File chrome/common/resource_usage_reporter.mojom (right): https://codereview.chromium.org/972083002/diff/180001/chrome/common/resource_... chrome/common/resource_usage_reporter.mojom:14: GetUsageData() => (ResourceUsageData data); please add a message/warning that for utility process, this only tracks v8 memory used by PAC
https://codereview.chromium.org/972083002/diff/180001/chrome/common/resource_... File chrome/common/resource_usage_reporter.mojom (right): https://codereview.chromium.org/972083002/diff/180001/chrome/common/resource_... chrome/common/resource_usage_reporter.mojom:14: GetUsageData() => (ResourceUsageData data); On 2015/05/19 16:11:47, jam wrote: > please add a message/warning that for utility process, this only tracks v8 > memory used by PAC Done.
The CQ bit was checked by amistry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, jam@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/972083002/#ps200001 (title: "Add comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972083002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/faa231a441d9890d8dee349b578dee7abed70501 Cr-Commit-Position: refs/heads/master@{#330670} |
