Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10828342:
Per-host V8 histograms. (Closed)
|
Created:
8 years, 4 months ago by marja Modified:
8 years, 3 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen (gone - plz use gerrit) Visibility:
Public. |
DescriptionPer-host V8 histograms.
Create custom V8 histograms for a specified list of hosts. The histograms are
used if all RenderViews are displaying a page from the same host.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154700
Patch Set 1 #Patch Set 2 : better version #
Total comments: 6
Patch Set 3 : new version #Patch Set 4 : . #
Total comments: 9
Patch Set 5 : code review #Patch Set 6 : rebased #
Total comments: 15
Patch Set 7 : Code review. #Patch Set 8 : tests & testability #
Total comments: 2
Patch Set 9 : test fix #
Total comments: 2
Patch Set 10 : fewer histograms #Patch Set 11 : maybe win build fix #Patch Set 12 : lame fix attempt for V8ValueConverterImplTest.BasicRoundTrip #
Messages
Total messages: 23 (0 generated)
http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_thr... content/renderer/render_thread_impl.cc:162: return ".gmail"; I don't understand how you're defining host. What is this data being used for? We generally use the notion of site (registered domain name plus protocol) because it's a conservative boundary on which pages might be able to script each other. In this case, you're missing the fact that subdomains might script each other (by modifying their document.domain), and you're putting HTTP and HTTPS into the same bucket (when they are considered different origins). http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_thr... File content/renderer/render_thread_impl.h (right): http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_thr... content/renderer/render_thread_impl.h:231: // RenderView's share (if any). If there is no common host, this function is This comment seems relevant for SetCommonHost(), not CommonHost(). http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_vie... content/renderer/render_view_impl.cc:1126: // some cases where the RenderViewImpls share a common host. This isn't safe. You'll miss renderer-initiated navigations (e.g., link clicks, frame navigations, etc), and it'll often be wrong due to redirects and downloads. It's much safer to watch commits instead.
Thanks for comments. (I'm not about to commit this CL even if it gets reviewed; we'd still need to check if this is the right thing to do. Just publishing a new version here in case you have more comments about the code...) http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_thr... content/renderer/render_thread_impl.cc:162: return ".gmail"; On 2012/08/16 16:22:56, creis wrote: > I don't understand how you're defining host. What is this data being used for? > > We generally use the notion of site (registered domain name plus protocol) > because it's a conservative boundary on which pages might be able to script each > other. In this case, you're missing the fact that subdomains might script each > other (by modifying their document.domain), and you're putting HTTP and HTTPS > into the same bucket (when they are considered different origins). Here we don't care about http vs. https. It would only make a difference if http://something would be a page different from https://something and we'd like to separate the histograms for them. For these hosts it's not the case. Also, if a page sets document.domain to be a suffix, we still use the original domain here, and ignore the suffix, and afaics it's good enough for this purpose. (If the user navigates to something.docs.com and that page sets its domain to docs.google.com, we lose that data, but we can probably live with that for now. I don't know if these properties even do that.) http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_thr... File content/renderer/render_thread_impl.h (right): http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_thr... content/renderer/render_thread_impl.h:231: // RenderView's share (if any). If there is no common host, this function is On 2012/08/16 16:22:56, creis wrote: > This comment seems relevant for SetCommonHost(), not CommonHost(). Done. http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/10828342/diff/1004/content/renderer/render_vie... content/renderer/render_view_impl.cc:1126: // some cases where the RenderViewImpls share a common host. On 2012/08/16 16:22:56, creis wrote: > This isn't safe. You'll miss renderer-initiated navigations (e.g., link clicks, > frame navigations, etc), and it'll often be wrong due to redirects and > downloads. It's much safer to watch commits instead. Changed this to do the work in RenderViewImpl::didCommitProvisionalLoad. It only tracks top frame navigations, and ignores subframes. We only take the top frame host into account when deciding whether to produce a custom diagram since the subframes are considered to be a part of the page.
http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... content/renderer/render_thread_impl.cc:170: std::string common_host; These are globals and should start with g_, right? Actually, can we avoid having them as globals? (There's only one render thread per process, right?) http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... content/renderer/render_thread_impl.cc:832: if (host != common_host) { Can we move some of the logic into here rather than at the call site? At the moment, we're relying on every caller to carefully check if there's still a common host. Seems like it might be a better contract to let this class listen for the hosts that are present and decide if there's a common one or not. http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... File content/renderer/render_thread_impl.h (right): http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... content/renderer/render_thread_impl.h:231: // empty string, if there is no common host. I think we need to say more about this in the comments and possibly improve the names, since it's not clear that this isn't intended for general use. (It has a very specific meaning, which includes hard-coded sites and V8 histograms.) Mainly, I wouldn't want other developers to think there's a supported concept of a render process's common host and try to use it for other purposes. http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... content/renderer/render_thread_impl.h:232: static std::string CommonHost(); This isn't virtual or complex, so it can probably be defined hacker_style() and defined inline.
Thanks for comments! http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... content/renderer/render_thread_impl.cc:170: std::string common_host; On 2012/08/21 22:46:49, creis wrote: > These are globals and should start with g_, right? Actually, can we avoid > having them as globals? (There's only one render thread per process, right?) They're inside an unnamed namespace, so only visible in this file and thus not globals. But I changed them to be member variables in the RenderThread instead. http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... content/renderer/render_thread_impl.cc:832: if (host != common_host) { On 2012/08/21 22:46:49, creis wrote: > Can we move some of the logic into here rather than at the call site? At the > moment, we're relying on every caller to carefully check if there's still a > common host. Seems like it might be a better contract to let this class listen > for the hosts that are present and decide if there's a common one or not. Done: Moved the logic to RenderThreadImpl. RenderViewImpl just calls RenderViewNavigatedToHost in RenderViewImpl::didCommitProvisionalLoad; is that ok? http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... File content/renderer/render_thread_impl.h (right): http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... content/renderer/render_thread_impl.h:231: // empty string, if there is no common host. On 2012/08/21 22:46:49, creis wrote: > I think we need to say more about this in the comments and possibly improve the > names, since it's not clear that this isn't intended for general use. (It has a > very specific meaning, which includes hard-coded sites and V8 histograms.) > > Mainly, I wouldn't want other developers to think there's a supported concept of > a render process's common host and try to use it for other purposes. Removed this function; added ConvertToCustomHistogramName now that the needed information is all inside RenderThreadImpl. http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... content/renderer/render_thread_impl.h:232: static std::string CommonHost(); On 2012/08/21 22:46:49, creis wrote: > This isn't virtual or complex, so it can probably be defined hacker_style() and > defined inline. This no longer exists either.
Thanks, this is much better. Can you add a unit test? It took me a bit to see that it's being used to generate historgrams like V8.GCCompactor.gmail. Otherwise I think it's almost ready. http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): http://codereview.chromium.org/10828342/diff/9001/content/renderer/render_thr... content/renderer/render_thread_impl.cc:832: if (host != common_host) { On 2012/08/22 14:59:31, marja wrote: > On 2012/08/21 22:46:49, creis wrote: > > Can we move some of the logic into here rather than at the call site? At the > > moment, we're relying on every caller to carefully check if there's still a > > common host. Seems like it might be a better contract to let this class > listen > > for the hosts that are present and decide if there's a common one or not. > > Done: Moved the logic to RenderThreadImpl. RenderViewImpl just calls > RenderViewNavigatedToHost in RenderViewImpl::didCommitProvisionalLoad; is that > ok? Yes, much better, thanks. http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... File content/renderer/render_thread_impl.cc (right): http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.cc:162: std::string HostToCustomPage(const std::string& host) { HostToCustomHistogramSuffix? http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.cc:833: // RenderViews. It's ok to not detect some cases where the RenderViews share a I'm glad you have this disclaimer. One case that comes to mind is opening a second render view in the same process before the first one commits, in which case you'll never have a common host, even if they both commit to the same host. That's fine (and likely rare), though. http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.cc:845: custom_histograms_.find(name) != custom_histograms_.end()) nit: Condition doesn't fit on one line, so needs braces. http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... File content/renderer/render_thread_impl.h (right): http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.h:242: // RenderViewThread's information about whether all RenderViews are displaying nit: RenderViewThread -> RenderThreadImpl? http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.h:341: // For producing custom histograms if all RenderViews share the same host. Can you say a bit more? e.g., that the histogram names in the set will be customized with a site-specific suffix if the process is only used for pages from a set of known sites? http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.h:343: std::string custom_page_; custom_page_ doesn't reveal much about what it's for. Maybe common_host_histogram_suffix_? http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:3215: frame->document().url()).host(), g_view_map.Get().size()); We should probably use GetLoadingURL, or move this to UpdateURL and use params.url.
Thanks again for comments! It seemed tricky to add a test (I couldn't just create a RenderThreadImpl in the unit tests because ChildThread ctor accessed message_loop_proxy and crashed, and not in browser test either, because ChildThread ctor again tried to create a IPC channel with file descriptor 0...), so I separated the histogram customizing functionality into an inner class RenderThreadImpl::HistogramCustomizer which can then be created without problems. http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... File content/renderer/render_thread_impl.cc (right): http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.cc:162: std::string HostToCustomPage(const std::string& host) { On 2012/08/22 23:09:59, creis wrote: > HostToCustomHistogramSuffix? Done. http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.cc:833: // RenderViews. It's ok to not detect some cases where the RenderViews share a On 2012/08/22 23:09:59, creis wrote: > I'm glad you have this disclaimer. One case that comes to mind is opening a > second render view in the same process before the first one commits, in which > case you'll never have a common host, even if they both commit to the same host. > That's fine (and likely rare), though. Ack. Yes, we don't need to catch all the possible cases where we could produce a custom histogram. http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.cc:845: custom_histograms_.find(name) != custom_histograms_.end()) On 2012/08/22 23:09:59, creis wrote: > nit: Condition doesn't fit on one line, so needs braces. counter-nit: Doesn't it only needs braces if the *body* doesn't fit on one line? http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... File content/renderer/render_thread_impl.h (right): http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.h:242: // RenderViewThread's information about whether all RenderViews are displaying On 2012/08/22 23:09:59, creis wrote: > nit: RenderViewThread -> RenderThreadImpl? Done. http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.h:341: // For producing custom histograms if all RenderViews share the same host. On 2012/08/22 23:09:59, creis wrote: > Can you say a bit more? e.g., that the histogram names in the set will be > customized with a site-specific suffix if the process is only used for pages > from a set of known sites? Done. http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.h:343: std::string custom_page_; On 2012/08/22 23:09:59, creis wrote: > custom_page_ doesn't reveal much about what it's for. Maybe > common_host_histogram_suffix_? Done. http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_vi... content/renderer/render_view_impl.cc:3215: frame->document().url()).host(), g_view_map.Get().size()); On 2012/08/22 23:09:59, creis wrote: > We should probably use GetLoadingURL, or move this to UpdateURL and use > params.url. Done.
Thanks for the test! LGTM, depending on your decision about whether closing all but one RenderView should mean there's a common host or not. (Memory-wise, it's unclear if everything from the other host will be reclaimed when closing its RenderView.) If that's not ok, you might need to track that state separately. http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... File content/renderer/render_thread_impl.cc (right): http://codereview.chromium.org/10828342/diff/17001/content/renderer/render_th... content/renderer/render_thread_impl.cc:845: custom_histograms_.find(name) != custom_histograms_.end()) On 2012/08/23 14:15:00, marja wrote: > On 2012/08/22 23:09:59, creis wrote: > > nit: Condition doesn't fit on one line, so needs braces. > > counter-nit: Doesn't it only needs braces if the *body* doesn't fit on one line? Ah, ambiguous style guides! :) Looks like there was a thread on this in November 2010: https://groups.google.com/a/chromium.org/forum/?fromgroups=#!searchin/chromiu... I was siding with Marc-Andre and Finnur, but it sounds like Brett and Peter argued that braces are optional for multiline conditionals, depending on readability. So you're right, they're not strictly needed! http://codereview.chromium.org/10828342/diff/20001/content/renderer/render_th... File content/renderer/render_thread_impl_unittest.cc (right): http://codereview.chromium.org/10828342/diff/20001/content/renderer/render_th... content/renderer/render_thread_impl_unittest.cc:73: // view navigated back to the common host. Are you ok with there being a custom histogram again if the second view is closed and the first view navigates? I think that will be considered a common host again.
Thanks for review! http://codereview.chromium.org/10828342/diff/20001/content/renderer/render_th... File content/renderer/render_thread_impl_unittest.cc (right): http://codereview.chromium.org/10828342/diff/20001/content/renderer/render_th... content/renderer/render_thread_impl_unittest.cc:73: // view navigated back to the common host. On 2012/08/23 16:30:40, creis wrote: > Are you ok with there being a custom histogram again if the second view is > closed and the first view navigates? I think that will be considered a common > host again. I think so; I'd assume that all resources from the second view, from V8 point of view, should be cleaned at that point. jochen, can you confirm this? (Another possibility would be to do the custom histograms only if there is only one render view, we'd probably get enough data from that, too...)
https://chromiumcodereview.appspot.com/10828342/diff/23001/content/renderer/r... File content/renderer/render_thread_impl.cc (right): https://chromiumcodereview.appspot.com/10828342/diff/23001/content/renderer/r... content/renderer/render_thread_impl.cc:212: custom_histograms_.insert("V8.MemoryExternalFragmentationTotal"); for now, I think the following three histograms are what we want: V8.MemoryExternalFragmentationTotal V8.MemoryHeapSampleTotalCommitted V8.MemoryHeapSampleTotalUsed
https://chromiumcodereview.appspot.com/10828342/diff/23001/content/renderer/r... File content/renderer/render_thread_impl.cc (right): https://chromiumcodereview.appspot.com/10828342/diff/23001/content/renderer/r... content/renderer/render_thread_impl.cc:212: custom_histograms_.insert("V8.MemoryExternalFragmentationTotal"); On 2012/08/28 12:48:54, jochen wrote: > for now, I think the following three histograms are what we want: > > V8.MemoryExternalFragmentationTotal > V8.MemoryHeapSampleTotalCommitted > V8.MemoryHeapSampleTotalUsed Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/10828342/26001
Try job failure for 10828342-26001 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/10828342/27007
Try job failure for 10828342-27007 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/10828342/27007
Try job failure for 10828342-27007 (retry) on linux_chromeos for step "content_unittests". It's a second try, previously, step "content_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/10828342/30009
Try job failure for 10828342-30009 (retry) (retry) on win_rel for step "browser_tests" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/10828342/30009
Try job failure for 10828342-30009 (retry) on linux_chromeos for step "runhooks". It's a second try, previously, step "content_browsertests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/10828342/30009
Change committed as 154700 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
