Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
315293003:
Add tracing for RenderView::hitTest and FrameView::invalidateTree. (Closed)
|
Created:
6 years, 6 months ago by chrishtr Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAdd tracing for RenderView::hitTest and FrameView::invalidateTree.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175635
Patch Set 1 #Patch Set 2 : Implement. #
Total comments: 5
Patch Set 3 : Remove redundant trace. #
Messages
Total messages: 15 (0 generated)
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrishtr@chromium.org/315293003/20001
https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1039: TRACE_EVENT0("blink", "FrameView::invalidateTree"); This already exists on line 1047.
The CQ bit was unchecked by chrishtr@chromium.org
https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1039: TRACE_EVENT0("blink", "FrameView::invalidateTree"); On 2014/06/06 01:59:03, dsinclair wrote: > This already exists on line 1047. Done.
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrishtr@chromium.org/315293003/40001
https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1039: TRACE_EVENT0("blink", "FrameView::invalidateTree"); Why is the one on line 1047 disabled by default?
https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1039: TRACE_EVENT0("blink", "FrameView::invalidateTree"); On 2014/06/06 03:22:38, ojan wrote: > Why is the one on line 1047 disabled by default? The TRACE_STR_COPY has a cost which I didn't want to incur in the default case.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
Message was sent while issue was closed.
Change committed as 175635
Message was sent while issue was closed.
https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:1039: TRACE_EVENT0("blink", "FrameView::invalidateTree"); On 2014/06/06 03:29:27, dsinclair wrote: > On 2014/06/06 03:22:38, ojan wrote: > > Why is the one on line 1047 disabled by default? > > The TRACE_STR_COPY has a cost which I didn't want to incur in the default case. Can we put in a trace (instead or in addition the the one below) that is enabled by default that doesn't add the root parameter? Having the trace by default is more useful than having the trace with the extra information. When we were looking at the trace today, there was this untraced block and we had to stare at the code and add speculative traces to see where the time was going.
Message was sent while issue was closed.
On 2014/06/06 05:53:11, ojan wrote: > https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... > File Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... > Source/core/frame/FrameView.cpp:1039: TRACE_EVENT0("blink", > "FrameView::invalidateTree"); > On 2014/06/06 03:29:27, dsinclair wrote: > > On 2014/06/06 03:22:38, ojan wrote: > > > Why is the one on line 1047 disabled by default? > > > > The TRACE_STR_COPY has a cost which I didn't want to incur in the default > case. > > Can we put in a trace (instead or in addition the the one below) that is enabled > by default that doesn't add the root parameter? Having the trace by default is > more useful than having the trace with the extra information. When we were > looking at the trace today, there was this untraced block and we had to stare at > the code and add speculative traces to see where the time was going. We can try just making the category: "blink," TRACE_DISABLED_BY_DEFAULT("blink.invalidation") (Hopefully that will do the right thing, otherwise "blink,disabled-by-default-blink.invalidation" which is a bit of a bummer.) The root parameter, at least for me, has been very useful a few times when the FrameView is doing a sub-layout. I wasn't able to figure that out until I added the root parameter.
Message was sent while issue was closed.
On 2014/06/06 at 13:25:26, dsinclair wrote: > On 2014/06/06 05:53:11, ojan wrote: > > https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... > > File Source/core/frame/FrameView.cpp (right): > > > > https://codereview.chromium.org/315293003/diff/20001/Source/core/frame/FrameV... > > Source/core/frame/FrameView.cpp:1039: TRACE_EVENT0("blink", > > "FrameView::invalidateTree"); > > On 2014/06/06 03:29:27, dsinclair wrote: > > > On 2014/06/06 03:22:38, ojan wrote: > > > > Why is the one on line 1047 disabled by default? > > > > > > The TRACE_STR_COPY has a cost which I didn't want to incur in the default > > case. > > > > Can we put in a trace (instead or in addition the the one below) that is enabled > > by default that doesn't add the root parameter? Having the trace by default is > > more useful than having the trace with the extra information. When we were > > looking at the trace today, there was this untraced block and we had to stare at > > the code and add speculative traces to see where the time was going. > > > We can try just making the category: > > "blink," TRACE_DISABLED_BY_DEFAULT("blink.invalidation") Lets do it! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
