Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2224063002:
Use persistent memory for receiving metrics from sub-processes. (Closed)
|
Created:
4 years, 4 months ago by bcwhite Modified:
4 years, 4 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse persistent memory for receiving metrics from sub-processes.
This allows metrics to survive and be collected even if a
subprocess terminates before reporting.
Only the GPU process is monitored with this CL but it should
be trivial to add the remaining ones going forward.
BUG=546019
Committed: https://crrev.com/b58a5fe78a1610d5c2667708cd235ab272614eec
Cr-Commit-Position: refs/heads/master@{#413452}
Patch Set 1 #Patch Set 2 : separate enable of tracking to not break with tests #Patch Set 3 : rebased #
Total comments: 20
Patch Set 4 : rebased #Patch Set 5 : addressed review comments by Alexei #
Total comments: 4
Patch Set 6 : create helper method; make test appear to be on UI thread #
Total comments: 4
Patch Set 7 : addressed final review comments by Alexei #Patch Set 8 : rebased #Messages
Total messages: 64 (44 generated)
The CQ bit was checked by bcwhite@chromium.org 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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by bcwhite@chromium.org 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 checked by bcwhite@chromium.org 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...
Patchset #2 (id:20001) has been deleted
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by bcwhite@chromium.org 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 checked by bcwhite@chromium.org 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...
Patchset #3 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by bcwhite@chromium.org 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 checked by bcwhite@chromium.org 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...
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Sorry, I likely won't be able to take a look at this until Friday as I still have full day training tomorrow. If you'd like a better turn around time than that, you could try another reviewer like isherman@ for now and I can take a look later.
https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/subprocess_metrics_provider.cc (right): https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider.cc:202: content::BrowserThread::PostTask( This seems overly complicated. I think you can simplify this via making unique_ptr<base::PersistentHistogramAllocator> the return value of this function and doing PostTaskAndReply() from the call site. I guess the downside is that early returns from lines 194 and 198 will still result in a reply with an empty unique ptr, but I think that's OK. https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider.cc:210: WrapUnique(new base::PersistentHistogramAllocator( Use base::MakeUnique. https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/subprocess_metrics_provider.h (right): https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider.h:41: // the correct thread can break tests. Can you provide more context? I would hope we can mock the current thread in tests so that we don't need to do this, but I might be missing something. https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... File content/browser/browser_child_process_host_impl.cc (right): https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... content/browser/browser_child_process_host_impl.cc:183: /*readonly=*/false)); I think it would be better to just have the the BrowserChildProcessHostImpl ctor take the base::SharedPersistentMemoryAllocator object, as opposed to 2 params. In that case, move this bit of logic to a helper function that can be invoked from the place where the ctor is being called. https://codereview.chromium.org/2224063002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2224063002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:61336: + Size, before padding, of objects allocated from persistent memory. This is For both of these histograms, mention something about the fact that it's for the GPU process.
The CQ bit was checked by bcwhite@chromium.org 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...
https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/subprocess_metrics_provider.cc (right): https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider.cc:202: content::BrowserThread::PostTask( On 2016/08/12 23:14:36, Alexei Svitkine (very slow) wrote: > This seems overly complicated. > > I think you can simplify this via making > unique_ptr<base::PersistentHistogramAllocator> the return value of this function > and doing PostTaskAndReply() from the call site. > > I guess the downside is that early returns from lines 194 and 198 will still > result in a reply with an empty unique ptr, but I think that's OK. Done. https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider.cc:210: WrapUnique(new base::PersistentHistogramAllocator( On 2016/08/12 23:14:36, Alexei Svitkine (very slow) wrote: > Use base::MakeUnique. Done. https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/subprocess_metrics_provider.h (right): https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider.h:41: // the correct thread can break tests. On 2016/08/12 23:14:36, Alexei Svitkine (very slow) wrote: > Can you provide more context? I would hope we can mock the current thread in > tests so that we don't need to do this, but I might be missing something. There's a DCHECK that it's running on BrowserThread::UI which is not true for tests. Unless there's a way for a test to so designate its thread? https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... File content/browser/browser_child_process_host_impl.cc (right): https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... content/browser/browser_child_process_host_impl.cc:183: /*readonly=*/false)); On 2016/08/12 23:14:36, Alexei Svitkine (very slow) wrote: > I think it would be better to just have the the BrowserChildProcessHostImpl ctor > take the base::SharedPersistentMemoryAllocator object, as opposed to 2 params. > > In that case, move this bit of logic to a helper function that can be invoked > from the place where the ctor is being called. I don't think that would be good. That would force the callers to know about persistent classes and the like which seems unnecessary when the only real things it has to provide is "how much" and "what's it called". https://codereview.chromium.org/2224063002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2224063002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:61336: + Size, before padding, of objects allocated from persistent memory. This is On 2016/08/12 23:14:37, Alexei Svitkine (very slow) wrote: > For both of these histograms, mention something about the fact that it's for the > GPU process. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/subprocess_metrics_provider.h (right): https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider.h:41: // the correct thread can break tests. On 2016/08/15 19:43:10, bcwhite wrote: > On 2016/08/12 23:14:36, Alexei Svitkine (very slow) wrote: > > Can you provide more context? I would hope we can mock the current thread in > > tests so that we don't need to do this, but I might be missing something. > > There's a DCHECK that it's running on BrowserThread::UI which is not true for > tests. > > Unless there's a way for a test to so designate its thread? See: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/qQiWjlxRcE4 https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... File content/browser/browser_child_process_host_impl.cc (right): https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... content/browser/browser_child_process_host_impl.cc:183: /*readonly=*/false)); On 2016/08/15 19:43:10, bcwhite wrote: > On 2016/08/12 23:14:36, Alexei Svitkine (very slow) wrote: > > I think it would be better to just have the the BrowserChildProcessHostImpl > ctor > > take the base::SharedPersistentMemoryAllocator object, as opposed to 2 params. > > > > In that case, move this bit of logic to a helper function that can be invoked > > from the place where the ctor is being called. > > I don't think that would be good. That would force the callers to know about > persistent classes and the like which seems unnecessary when the only real > things it has to provide is "how much" and "what's it called". The callers wouldn't need to know the persistent classes if they use a helper function. The current implementation makes this class know about all the details of how to create a SharedPersistentMemoryAllocator, whereas this seems like a detail that this class shouldn't concern itself with. How about making a helper function that takes "how much" and "what it's called" and returns the allocator - and that function can be called at the callsites or nullptr be passed. The function can live somewhere in persistent metrics code, so only that code needs to know the details of how to set it up.
https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... File content/browser/browser_child_process_host_impl.cc (right): https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... content/browser/browser_child_process_host_impl.cc:183: /*readonly=*/false)); > > I don't think that would be good. That would force the callers to know about > > persistent classes and the like which seems unnecessary when the only real > > things it has to provide is "how much" and "what's it called". > > The callers wouldn't need to know the persistent classes if they use a helper > function. The current implementation makes this class know about all the details > of how to create a SharedPersistentMemoryAllocator, whereas this seems like a > detail that this class shouldn't concern itself with. This class knows about creating shared memory because this class is what passes that shared memory to the newly created process. Creating an allocator on top of it already does use a helper-function (actually a helper-class) named SharedPersistentMemoryAllocator. > How about making a helper function that takes "how much" and "what it's called" > and returns the allocator - and that function can be called at the callsites or > nullptr be passed. The function can live somewhere in persistent metrics code, > so only that code needs to know the details of how to set it up. This class needs to know about an allocator (so it can give it away) and how it's based on shared memory (so it can pass that memory to the child process). There doesn't seem to be any benefit in spreading that knowledge and their inter-relationship to additional modules. This is also the only place that knows the process ID (line 150) which is used as an identifier inside the memory allocator -- nothing passing in an allocator as a parameter could get that because it hasn't yet been determined.
https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... File content/browser/browser_child_process_host_impl.cc (right): https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... content/browser/browser_child_process_host_impl.cc:183: /*readonly=*/false)); On 2016/08/18 15:21:55, bcwhite wrote: > > > I don't think that would be good. That would force the callers to know > about > > > persistent classes and the like which seems unnecessary when the only real > > > things it has to provide is "how much" and "what's it called". > > > > The callers wouldn't need to know the persistent classes if they use a helper > > function. The current implementation makes this class know about all the > details > > of how to create a SharedPersistentMemoryAllocator, whereas this seems like a > > detail that this class shouldn't concern itself with. > > This class knows about creating shared memory because this class is what passes > that shared memory to the newly created process. Creating an allocator on top > of it already does use a helper-function (actually a helper-class) named > SharedPersistentMemoryAllocator. Let me step back a bit. The concrete things I'm not a fan of with the current proposed implementation: 1. The block of code in BrowserChildProcessHostImpl cares a lot about the persistent memory details. I'd rather it be a helper function that just takes id, metrics memory name and size and return it. 2. The magic numbers introduced in callers of the ctor - i.e. the code in gpu_process_host.cc now has magic constants (name and size in it). Both of those make the existing code that currently doesn't need to care very much about the persistent memory stuff be deeply involved with it. I'd rather not have us dilute the focus of those files. Could we perhaps just introduce a "child_process_persistent_memory.cc" file that has a CreatePersistentMemory(content::ProcessType process_type) function? Then, that function could have the full logic - including switching on the process type to get the name/size? If you add such a function, then maybe you don't even need any change to the ctor signature - since it can be called directly from the ctor impl. > > > How about making a helper function that takes "how much" and "what it's > called" > > and returns the allocator - and that function can be called at the callsites > or > > nullptr be passed. The function can live somewhere in persistent metrics code, > > so only that code needs to know the details of how to set it up. > > This class needs to know about an allocator (so it can give it away) and how > it's based on shared memory (so it can pass that memory to the child process). > There doesn't seem to be any benefit in spreading that knowledge and their > inter-relationship to additional modules. This is also the only place that > knows the process ID (line 150) which is used as an identifier inside the memory > allocator -- nothing passing in an allocator as a parameter could get that > because it hasn't yet been determined. https://codereview.chromium.org/2224063002/diff/160001/chrome/browser/metrics... File chrome/browser/metrics/subprocess_metrics_provider.cc (right): https://codereview.chromium.org/2224063002/diff/160001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider.cc:194: SubprocessMetricsProvider::GetSubprocessHistogramAllocatorOnIOThread(int id) { Nit: Make this a free-standing function in the anon namespace. https://codereview.chromium.org/2224063002/diff/160001/content/browser/browse... File content/browser/browser_child_process_host_impl.h (right): https://codereview.chromium.org/2224063002/diff/160001/content/browser/browse... content/browser/browser_child_process_host_impl.h:124: // requires the process to have been launched and the RPC channel to be Nit: RPC -> IPC (I think IPC is the term we use in Chromium code)
https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... File content/browser/browser_child_process_host_impl.cc (right): https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... content/browser/browser_child_process_host_impl.cc:183: /*readonly=*/false)); > Let me step back a bit. The concrete things I'm not a fan of with the current > proposed implementation: > > 1. The block of code in BrowserChildProcessHostImpl cares a lot about the > persistent memory details. I'd rather it be a helper function that just takes > id, metrics memory name and size and return it. It cares about the shared memory details but that's because it is what is sharing the memory. Since this class is doing the sharing and doing the holding and the passing to whatever "takes" it, it seems wrong to move the creation of it somewhere else. Creating the allocator on top of it is not significant enough to be separated. > 2. The magic numbers introduced in callers of the ctor - i.e. the code in > gpu_process_host.cc now has magic constants (name and size in it). > > Both of those make the existing code that currently doesn't need to care very > much about the persistent memory stuff be deeply involved with it. I'd rather > not have us dilute the focus of those files. I thought it would be better to keep constants related to a specific process type with that specific process type class. However, if you'd prefer it all in one place, I can write that static helper function inside this class -- no need for a new file.
Sorry for all the delays reviewing this. I was traveling yesterday and was at a summit (which took whole day) on Tue/Wed. And today I'm working just this morning before going on vacation this afternoon and all of next week. :( Looking again now (will reply in a bit), but likely you'll need to loop in another reviewer to finish this.
https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/subprocess_metrics_provider.h (right): https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider.h:41: // the correct thread can break tests. On 2016/08/18 07:21:19, Alexei Svitkine (OOO) wrote: > On 2016/08/15 19:43:10, bcwhite wrote: > > On 2016/08/12 23:14:36, Alexei Svitkine (very slow) wrote: > > > Can you provide more context? I would hope we can mock the current thread in > > > tests so that we don't need to do this, but I might be missing something. > > > > There's a DCHECK that it's running on BrowserThread::UI which is not true for > > tests. > > > > Unless there's a way for a test to so designate its thread? > > See: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/qQiWjlxRcE4 Ping on this. I think you should be able to use the techniques mention in this thread to avoid the separate function here. https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... File content/browser/browser_child_process_host_impl.cc (right): https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... content/browser/browser_child_process_host_impl.cc:183: /*readonly=*/false)); On 2016/08/18 17:42:17, bcwhite wrote: > > Let me step back a bit. The concrete things I'm not a fan of with the current > > proposed implementation: > > > > 1. The block of code in BrowserChildProcessHostImpl cares a lot about the > > persistent memory details. I'd rather it be a helper function that just takes > > id, metrics memory name and size and return it. > > It cares about the shared memory details but that's because it is what is > sharing the memory. Since this class is doing the sharing and doing the holding > and the passing to whatever "takes" it, it seems wrong to move the creation of > it somewhere else. > > Creating the allocator on top of it is not significant enough to be separated. I see your point. I still don't like that this class needs to know about the GlobalHistogramAllocator object not being null. And it actually seems error prone to require callers to know this - what happens if someone forgets to make that check somehow? Perhaps it would make more sense to have a static factory function to create a SharedPersistentMemoryAllocator? This could live in the base metrics code and just take the id, name and size as params. Then, this block would be much simpler: metrics_allocator_.reset(base::SharedPersistentMemoryAllocator::Create(shared_metrics_name, shared_metrics_memory_size, static_cast<uint64_t>(data_.id)); And that create function will do all the details such as checking size and presence of the global allocator and creating SharedMemory instance. This way, this class needs to know much less details of the persistent memory implementation. > > > > 2. The magic numbers introduced in callers of the ctor - i.e. the code in > > gpu_process_host.cc now has magic constants (name and size in it). > > > > Both of those make the existing code that currently doesn't need to care very > > much about the persistent memory stuff be deeply involved with it. I'd rather > > not have us dilute the focus of those files. > > I thought it would be better to keep constants related to a specific process > type with that specific process type class. However, if you'd prefer it all in > one place, I can write that static helper function inside this class -- no need > for a new file. Thinking about this more - do we really need the name as a param? I think we can just use GetProcessTypeNameInEnglish() on the process type and use that. (And maybe preprocess that to remove spaces.) Then, that simplifies the ctor API some. As to keeping the size param with the callers, I guess that part is OK. Maybe instead of inlining it, which makes it very cryptic, pull it out into a constant at the top of each file with a relevant comment.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #6 (id:180001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/subprocess_metrics_provider.h (right): https://codereview.chromium.org/2224063002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider.h:41: // the correct thread can break tests. On 2016/08/19 15:10:51, Alexei Svitkine (OOO) wrote: > On 2016/08/18 07:21:19, Alexei Svitkine (OOO) wrote: > > On 2016/08/15 19:43:10, bcwhite wrote: > > > On 2016/08/12 23:14:36, Alexei Svitkine (very slow) wrote: > > > > Can you provide more context? I would hope we can mock the current thread > in > > > > tests so that we don't need to do this, but I might be missing something. > > > > > > There's a DCHECK that it's running on BrowserThread::UI which is not true > for > > > tests. > > > > > > Unless there's a way for a test to so designate its thread? > > > > See: > > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/qQiWjlxRcE4 > > Ping on this. I think you should be able to use the techniques mention in this > thread to avoid the separate function here. Yep. Was just addressing the design issues quickly before investigating. Done. https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... File content/browser/browser_child_process_host_impl.cc (right): https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... content/browser/browser_child_process_host_impl.cc:183: /*readonly=*/false)); New separate method for creating the allocator that determines parameters based on process type rather than them being passed in. https://codereview.chromium.org/2224063002/diff/120001/content/browser/browse... content/browser/browser_child_process_host_impl.cc:183: /*readonly=*/false)); > > It cares about the shared memory details but that's because it is what is > > sharing the memory. Since this class is doing the sharing and doing the > holding > > and the passing to whatever "takes" it, it seems wrong to move the creation of > > it somewhere else. > > > > Creating the allocator on top of it is not significant enough to be separated. > > I see your point. I still don't like that this class needs to know about the > GlobalHistogramAllocator object not being null. And it actually seems error > prone to require callers to know this - what happens if someone forgets to make > that check somehow? Eventually that will be removed, but it's used here and elsewhere instead of checking for the existence of the field-trial. I'll add a TODO to remove it. > > I thought it would be better to keep constants related to a specific process > > type with that specific process type class. However, if you'd prefer it all > in > > one place, I can write that static helper function inside this class -- no > need > > for a new file. > > Thinking about this more - do we really need the name as a param? I think we can > just use GetProcessTypeNameInEnglish() on the process type and use that. (And > maybe preprocess that to remove spaces.) I suppose. Could those names ever change? It would break the metrics reporting if they did but the histograms.xml file wasn't updated to match. > As to keeping the size param with the callers, I guess that part is OK. Maybe > instead of inlining it, which makes it very cryptic, pull it out into a constant > at the top of each file with a relevant comment. Heh. I came to your way of thinking that embedding all that in a single place would keep the API simple and be easier to maintain. :-)
LGTM, thanks! https://codereview.chromium.org/2224063002/diff/160001/content/browser/browse... File content/browser/browser_child_process_host_impl.h (right): https://codereview.chromium.org/2224063002/diff/160001/content/browser/browse... content/browser/browser_child_process_host_impl.h:124: // requires the process to have been launched and the RPC channel to be On 2016/08/18 15:57:28, Alexei Svitkine (OOO) wrote: > Nit: RPC -> IPC (I think IPC is the term we use in Chromium code) Ping. https://codereview.chromium.org/2224063002/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/subprocess_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2224063002/diff/200001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider_unittest.cc:122: // A thread-buldle makes the tests appear on the UI thread, something that is Nit: buldle -> bundle https://codereview.chromium.org/2224063002/diff/200001/content/browser/browse... File content/browser/browser_child_process_host_impl.h (right): https://codereview.chromium.org/2224063002/diff/200001/content/browser/browse... content/browser/browser_child_process_host_impl.h:18: #include "base/strings/string_piece.h" Nit: Not needed anymore.
The CQ bit was checked by bcwhite@chromium.org 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...
https://codereview.chromium.org/2224063002/diff/160001/content/browser/browse... File content/browser/browser_child_process_host_impl.h (right): https://codereview.chromium.org/2224063002/diff/160001/content/browser/browse... content/browser/browser_child_process_host_impl.h:124: // requires the process to have been launched and the RPC channel to be On 2016/08/19 16:31:56, Alexei Svitkine (OOO) wrote: > On 2016/08/18 15:57:28, Alexei Svitkine (OOO) wrote: > > Nit: RPC -> IPC (I think IPC is the term we use in Chromium code) > > Ping. Done. https://codereview.chromium.org/2224063002/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/subprocess_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2224063002/diff/200001/chrome/browser/metrics... chrome/browser/metrics/subprocess_metrics_provider_unittest.cc:122: // A thread-buldle makes the tests appear on the UI thread, something that is On 2016/08/19 16:31:56, Alexei Svitkine (OOO) wrote: > Nit: buldle -> bundle Done. https://codereview.chromium.org/2224063002/diff/200001/content/browser/browse... File content/browser/browser_child_process_host_impl.h (right): https://codereview.chromium.org/2224063002/diff/200001/content/browser/browse... content/browser/browser_child_process_host_impl.h:18: #include "base/strings/string_piece.h" On 2016/08/19 16:31:56, Alexei Svitkine (OOO) wrote: > Nit: Not needed anymore. Done.
The CQ bit was unchecked by bcwhite@chromium.org
bcwhite@chromium.org changed reviewers: + avi@chromium.org
avi@chromium.org: Please review changes in browser_child_process_host*
lgtm
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2224063002/#ps220001 (title: "addressed final review comments by Alexei")
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2224063002/#ps240001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Use persistent memory for receiving metrics from sub-processes. This allows metrics to survive and be collected even if a subprocess terminates before reporting. Only the GPU process is monitored with this CL but it should be trivial to add the remaining ones going forward. BUG=546019 ========== to ========== Use persistent memory for receiving metrics from sub-processes. This allows metrics to survive and be collected even if a subprocess terminates before reporting. Only the GPU process is monitored with this CL but it should be trivial to add the remaining ones going forward. BUG=546019 Committed: https://crrev.com/b58a5fe78a1610d5c2667708cd235ab272614eec Cr-Commit-Position: refs/heads/master@{#413452} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b58a5fe78a1610d5c2667708cd235ab272614eec Cr-Commit-Position: refs/heads/master@{#413452} |
