Move paint-related timings out of DocumentTiming

Move paint-related timings out of DocumentTiming

This patch moves paint related timings of DocumentTiming into a new
class, PaintTiming, which is a document supplement and created on demand
only for visible documents.

BUG=544811
Committed: https://crrev.com/ad9c27b059c5d37dc278e0c7a4b8cdb202952491
Cr-Commit-Position: refs/heads/master@{#361307}

[Kunihiko Sakamoto, 2015-11-19 03:53:45]

Description was changed from

==========
WIP
==========

to

==========
Move paint-related timings out of DocumentTiming

This patch moves paint related timings of DocumentTiming into a new
class, PaintTiming, which is a document supplement and created on demand
only for visible documents.

BUG=544811
==========

[Kunihiko Sakamoto, 2015-11-19 03:53:58]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2015-11-19 04:30:33]

ksakamoto@chromium.org changed reviewers:
+ chrishtr@chromium.org, dglazkov@chromium.org, kinuko@chromium.org

[Kunihiko Sakamoto, 2015-11-19 04:30:33]

PTAL

Still PaintTiming belongs to Document (as a supplement) and
PaintLayer::paintTiming() is reaching into Document, but I guess it's ok as
PaintLayer uses Document in several other places too...

[dglazkov, 2015-11-19 05:13:11]

On 2015/11/19 at 04:30:33, ksakamoto wrote:
> PTAL
> 
> Still PaintTiming belongs to Document (as a supplement) and
PaintLayer::paintTiming() is reaching into Document, but I guess it's ok as
PaintLayer uses Document in several other places too...

I am confused why it still sits on Document, and not on something more Paint-ey.
Chris, is there a more appropriate object to own poor ole PaintTiming?

[chrishtr, 2015-11-20 00:50:16]

On 2015/11/19 at 05:13:11, dglazkov wrote:
> On 2015/11/19 at 04:30:33, ksakamoto wrote:
> > PTAL
> > 
> > Still PaintTiming belongs to Document (as a supplement) and
PaintLayer::paintTiming() is reaching into Document, but I guess it's ok as
PaintLayer uses Document in several other places too...
> 
> I am confused why it still sits on Document, and not on something more
Paint-ey. Chris, is there a more appropriate object to own poor ole PaintTiming?

Is the desired place a per-frame thing or global to the webview? A little
confused about that point.

[dglazkov, 2015-11-20 03:43:53]

On 2015/11/20 at 00:50:16, chrishtr wrote:
> On 2015/11/19 at 05:13:11, dglazkov wrote:
> > On 2015/11/19 at 04:30:33, ksakamoto wrote:
> > > PTAL
> > > 
> > > Still PaintTiming belongs to Document (as a supplement) and
PaintLayer::paintTiming() is reaching into Document, but I guess it's ok as
PaintLayer uses Document in several other places too...
> > 
> > I am confused why it still sits on Document, and not on something more
Paint-ey. Chris, is there a more appropriate object to own poor ole PaintTiming?
> 
> Is the desired place a per-frame thing or global to the webview? A little
confused about that point.

Per-top-frame-navigation, I think.

Oh hey, that seems like it should LayoutView. Can we hoist it on LayoutView?

[kinuko (google), 2015-11-20 04:59:26]

On 2015/11/20 03:43:53, dglazkov wrote:
> On 2015/11/20 at 00:50:16, chrishtr wrote:
> > On 2015/11/19 at 05:13:11, dglazkov wrote:
> > > On 2015/11/19 at 04:30:33, ksakamoto wrote:
> > > > PTAL
> > > > 
> > > > Still PaintTiming belongs to Document (as a supplement) and
> PaintLayer::paintTiming() is reaching into Document, but I guess it's ok as
> PaintLayer uses Document in several other places too...
> > > 
> > > I am confused why it still sits on Document, and not on something more
> Paint-ey. Chris, is there a more appropriate object to own poor ole
PaintTiming?
> > 
> > Is the desired place a per-frame thing or global to the webview? A little
> confused about that point.
> 
> Per-top-frame-navigation, I think.
> 
> Oh hey, that seems like it should LayoutView. Can we hoist it on LayoutView?

Basically what we want to do is to gather metrics and notify them to the content
layer (to forward them to the browser) per top-frame-navigation, yes.  It's
probably not super necessary to make the timing data go through DocumentLoader
or DocumentLoadTiming (while currently it's the only place we notify timing
updates to the render frame observer), as most aggregation logic is now in the
browser process.

(sakamoto-san, you might want to get feedback from csharrison@ too if we're
changing or adding the notification handling)

[chrishtr, 2015-11-20 18:11:27]

On 2015/11/20 at 04:59:26, kinuko wrote:
> On 2015/11/20 03:43:53, dglazkov wrote:
> > On 2015/11/20 at 00:50:16, chrishtr wrote:
> > > On 2015/11/19 at 05:13:11, dglazkov wrote:
> > > > On 2015/11/19 at 04:30:33, ksakamoto wrote:
> > > > > PTAL
> > > > > 
> > > > > Still PaintTiming belongs to Document (as a supplement) and
> > PaintLayer::paintTiming() is reaching into Document, but I guess it's ok as
> > PaintLayer uses Document in several other places too...
> > > > 
> > > > I am confused why it still sits on Document, and not on something more
> > Paint-ey. Chris, is there a more appropriate object to own poor ole
PaintTiming?
> > > 
> > > Is the desired place a per-frame thing or global to the webview? A little
> > confused about that point.
> > 
> > Per-top-frame-navigation, I think.
> > 
> > Oh hey, that seems like it should LayoutView. Can we hoist it on LayoutView?
> 
> Basically what we want to do is to gather metrics and notify them to the
content layer (to forward them to the browser) per top-frame-navigation, yes. 
It's probably not super necessary to make the timing data go through
DocumentLoader or DocumentLoadTiming (while currently it's the only place we
notify timing updates to the render frame observer), as most aggregation logic
is now in the browser process.
> 
> (sakamoto-san, you might want to get feedback from csharrison@ too if we're
changing or adding the notification handling)

Is it desired to store stuff independently per frame for situations such as
cross-domain iframes?

In any case, the paint-y thing is PaintController, which is per-composited
layer. I don't think you want to store final info there. There is also
PaintLayerCompositor, which is per-frame, but that is actually being refactored
away so it would be nice to avoid it.

Once slimming paint is far enough along, there will be one PaintController per
top-level view (or when OOPIF is present, connected subset of the frame tree
that is local to this process).

In the meantime, it seems reasonable enough to store the information where it is
in this CL.

[Bryan McQuade, 2015-11-20 18:13:19]

On 2015/11/20 18:11:27, chrishtr wrote:
> On 2015/11/20 at 04:59:26, kinuko wrote:
> > On 2015/11/20 03:43:53, dglazkov wrote:
> > > On 2015/11/20 at 00:50:16, chrishtr wrote:
> > > > On 2015/11/19 at 05:13:11, dglazkov wrote:
> > > > > On 2015/11/19 at 04:30:33, ksakamoto wrote:
> > > > > > PTAL
> > > > > > 
> > > > > > Still PaintTiming belongs to Document (as a supplement) and
> > > PaintLayer::paintTiming() is reaching into Document, but I guess it's ok
as
> > > PaintLayer uses Document in several other places too...
> > > > > 
> > > > > I am confused why it still sits on Document, and not on something more
> > > Paint-ey. Chris, is there a more appropriate object to own poor ole
> PaintTiming?
> > > > 
> > > > Is the desired place a per-frame thing or global to the webview? A
little
> > > confused about that point.
> > > 
> > > Per-top-frame-navigation, I think.
> > > 
> > > Oh hey, that seems like it should LayoutView. Can we hoist it on
LayoutView?
> > 
> > Basically what we want to do is to gather metrics and notify them to the
> content layer (to forward them to the browser) per top-frame-navigation, yes. 
> It's probably not super necessary to make the timing data go through
> DocumentLoader or DocumentLoadTiming (while currently it's the only place we
> notify timing updates to the render frame observer), as most aggregation logic
> is now in the browser process.
> > 
> > (sakamoto-san, you might want to get feedback from csharrison@ too if we're
> changing or adding the notification handling)
> 
> Is it desired to store stuff independently per frame for situations such as
> cross-domain iframes?
> 
> In any case, the paint-y thing is PaintController, which is per-composited
> layer. I don't think you want to store final info there. There is also
> PaintLayerCompositor, which is per-frame, but that is actually being
refactored
> away so it would be nice to avoid it.
> 
> Once slimming paint is far enough along, there will be one PaintController per
> top-level view (or when OOPIF is present, connected subset of the frame tree
> that is local to this process).
> 
> In the meantime, it seems reasonable enough to store the information where it
is
> in this CL.

If it doesn't add substantial complexity, I'd like to track this on a per-frame
basis, for reasons like tracking performance of iframes.

If it does add complexity in the near term, tracking it for the top level frame
only seems ok for now. I'd like to work towards a solution that allows us to
track this per frame in the future, though.

[dglazkov, 2015-11-20 18:16:40]

On 2015/11/20 at 18:11:27, chrishtr wrote:
> On 2015/11/20 at 04:59:26, kinuko wrote:
> > On 2015/11/20 03:43:53, dglazkov wrote:
> > > On 2015/11/20 at 00:50:16, chrishtr wrote:
> > > > On 2015/11/19 at 05:13:11, dglazkov wrote:
> > > > > On 2015/11/19 at 04:30:33, ksakamoto wrote:
> > > > > > PTAL
> > > > > > 
> > > > > > Still PaintTiming belongs to Document (as a supplement) and
> > > PaintLayer::paintTiming() is reaching into Document, but I guess it's ok
as
> > > PaintLayer uses Document in several other places too...
> > > > > 
> > > > > I am confused why it still sits on Document, and not on something more
> > > Paint-ey. Chris, is there a more appropriate object to own poor ole
PaintTiming?
> > > > 
> > > > Is the desired place a per-frame thing or global to the webview? A
little
> > > confused about that point.
> > > 
> > > Per-top-frame-navigation, I think.
> > > 
> > > Oh hey, that seems like it should LayoutView. Can we hoist it on
LayoutView?
> > 
> > Basically what we want to do is to gather metrics and notify them to the
content layer (to forward them to the browser) per top-frame-navigation, yes. 
It's probably not super necessary to make the timing data go through
DocumentLoader or DocumentLoadTiming (while currently it's the only place we
notify timing updates to the render frame observer), as most aggregation logic
is now in the browser process.
> > 
> > (sakamoto-san, you might want to get feedback from csharrison@ too if we're
changing or adding the notification handling)
> 
> Is it desired to store stuff independently per frame for situations such as
cross-domain iframes?
> 
> In any case, the paint-y thing is PaintController, which is per-composited
layer. I don't think you want to store final info there. There is also
PaintLayerCompositor, which is per-frame, but that is actually being refactored
away so it would be nice to avoid it.
> 
> Once slimming paint is far enough along, there will be one PaintController per
top-level view (or when OOPIF is present, connected subset of the frame tree
that is local to this process).
> 
> In the meantime, it seems reasonable enough to store the information where it
is in this CL.

Makes sense, lgtm.

[chrishtr, 2015-11-20 18:17:14]

On 2015/11/20 at 18:13:19, bmcquade wrote:
> On 2015/11/20 18:11:27, chrishtr wrote:
> > On 2015/11/20 at 04:59:26, kinuko wrote:
> > > On 2015/11/20 03:43:53, dglazkov wrote:
> > > > On 2015/11/20 at 00:50:16, chrishtr wrote:
> > > > > On 2015/11/19 at 05:13:11, dglazkov wrote:
> > > > > > On 2015/11/19 at 04:30:33, ksakamoto wrote:
> > > > > > > PTAL
> > > > > > > 
> > > > > > > Still PaintTiming belongs to Document (as a supplement) and
> > > > PaintLayer::paintTiming() is reaching into Document, but I guess it's ok
as
> > > > PaintLayer uses Document in several other places too...
> > > > > > 
> > > > > > I am confused why it still sits on Document, and not on something
more
> > > > Paint-ey. Chris, is there a more appropriate object to own poor ole
> > PaintTiming?
> > > > > 
> > > > > Is the desired place a per-frame thing or global to the webview? A
little
> > > > confused about that point.
> > > > 
> > > > Per-top-frame-navigation, I think.
> > > > 
> > > > Oh hey, that seems like it should LayoutView. Can we hoist it on
LayoutView?
> > > 
> > > Basically what we want to do is to gather metrics and notify them to the
> > content layer (to forward them to the browser) per top-frame-navigation,
yes. 
> > It's probably not super necessary to make the timing data go through
> > DocumentLoader or DocumentLoadTiming (while currently it's the only place we
> > notify timing updates to the render frame observer), as most aggregation
logic
> > is now in the browser process.
> > > 
> > > (sakamoto-san, you might want to get feedback from csharrison@ too if
we're
> > changing or adding the notification handling)
> > 
> > Is it desired to store stuff independently per frame for situations such as
> > cross-domain iframes?
> > 
> > In any case, the paint-y thing is PaintController, which is per-composited
> > layer. I don't think you want to store final info there. There is also
> > PaintLayerCompositor, which is per-frame, but that is actually being
refactored
> > away so it would be nice to avoid it.
> > 
> > Once slimming paint is far enough along, there will be one PaintController
per
> > top-level view (or when OOPIF is present, connected subset of the frame tree
> > that is local to this process).
> > 
> > In the meantime, it seems reasonable enough to store the information where
it is
> > in this CL.
> 
> If it doesn't add substantial complexity, I'd like to track this on a
per-frame basis, for reasons like tracking performance of iframes.
> 
> If it does add complexity in the near term, tracking it for the top level
frame only seems ok for now. I'd like to work towards a solution that allows us
to track this per frame in the future, though.

Tracking per-frame is hard actually, in the case when a frame is not
individually composited. In such a situation there is really only one
PaintController for the entire top-level page, and by design it's not possible
to ask the PaintController which text was painted in which frame.

[Kunihiko Sakamoto, 2015-11-24 06:01:48]

Thank you all for the comments.
Let me commit this, and keep the tracking bug open for the possible future plan
Chris mentioned in #9.

[Kunihiko Sakamoto, 2015-11-24 06:02:21]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2015-11-24 06:04:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456883002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456883002/20001

[commit-bot: I haz the power, 2015-11-24 08:09:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-24 08:09:27]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Kunihiko Sakamoto, 2015-11-24 08:11:49]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2015-11-24 08:12:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456883002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456883002/20001

[commit-bot: I haz the power, 2015-11-24 10:36:32]

Committed patchset #1 (id:20001)

[commit-bot: I haz the power, 2015-11-24 10:37:30]

Patchset 1 (id:??) landed as
https://crrev.com/ad9c27b059c5d37dc278e0c7a4b8cdb202952491
Cr-Commit-Position: refs/heads/master@{#361307}

[Mostyn Bramley-Moore, 2015-11-24 11:24:05]

This seems to have broken GCC builds (I work on a downstream embedded linux
project, but I believe some official chromeos builds still use GCC):

FAILED: ccache g++ -MMD -MF
obj/third_party/WebKit/Source/core/paint/webcore_shared.PaintTiming.o.d
-DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -D_FILE_OFFSET_BITS=64 -DDISABLE_NACL
-DCHROMIUM_BUILD -DCOMPONENT_BUILD -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1
-DUSE_ASH=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_DEFAULT_RENDER_THEME=1
-DUSE_LIBJPEG_TURBO=1 -DUSE_X11=1 -DUSE_CLIPBOARD_AURAX11=1
-DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP -DENABLE_WEBRTC=1
-DENABLE_MEDIA_ROUTER=1 -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY
-DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_TOPCHROME_MD=1
-DDONT_EMBED_BUILD_METADATA -DDCHECK_ALWAYS_ON=1 -DFIELDTRIAL_TESTING_ENABLED
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGINS=1
-DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1
-DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD
-DSAFE_BROWSING_DB_LOCAL -DBLINK_CORE_IMPLEMENTATION=1 -DBLINK_IMPLEMENTATION=1
-DINSIDE_BLINK -DGL_GLEXT_PROTOTYPES -DCHROME_PNG_WRITE_SUPPORT
-DPNG_USER_CONFIG -DUSE_SYSTEM_LIBJPEG -DENABLE_LAYOUT_UNIT_IN_INLINE_BOXES=0
-DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1
-DENABLE_WEB_AUDIO=1 -DWTF_USE_WEBAUDIO_FFMPEG=1
-DWTF_USE_DEFAULT_RENDER_THEME=1 -DLINK_CORE_MODULES_SEPARATELY
-DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DSKIA_DLL
-DGR_GL_IGNORE_ES3_MSAA=0 -DSK_SUPPORT_GPU=1
-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_IGNORE_GL_TEXTURE_TARGET
-DUSE_SYSTEM_LIBXML -DV8_SHARED -DUSING_V8_SHARED -DUSE_LIBPCI=1 -DUSE_OPENSSL=1
-DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen
-I../../third_party/WebKit/Source -Igen/blink -I../../third_party/WebKit
-I../../third_party/WebKit/Source/core/testing
-I../../third_party/WebKit/Source/core/testing/v8 -I../.. -I../../skia/config
-I../../third_party/khronos -I../../gpu -Igen/angle
-I../../third_party/angle/include -I../../third_party/ffmpeg
-I../../third_party/iccjpeg -I../../third_party/libpng
-I../../third_party/libwebp -I../../third_party/ots/include
-I../../third_party/zlib -I../../third_party/icu/source/i18n
-I../../third_party/icu/source/common -I../../skia/ext
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils -I../../third_party/npapi
-I../../third_party/npapi/bindings -I../../third_party/qcms/src
-I../../third_party/snappy/linux -I../../third_party/snappy/src
-I../../v8/include -fstack-protector --param=ssp-buffer-size=4 -Werror -pthread
-fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers
-fvisibility=hidden -pipe -fPIC -Wno-unused-local-typedefs -fno-strict-aliasing
-I/usr/include/freetype2 -pthread -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/libxml2
-I/usr/include/libxml2 -m64 -march=x86-64 -O2 -fno-ident -fdata-sections
-ffunction-sections -funwind-tables -fno-exceptions -fno-rtti
-fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare
-Wno-c++0x-compat -std=gnu++11 -Wno-narrowing -Wno-literal-suffix  -c
../../third_party/WebKit/Source/core/paint/PaintTiming.cpp -o
obj/third_party/WebKit/Source/core/paint/webcore_shared.PaintTiming.o
In file included from
../../third_party/WebKit/Source/core/paint/PaintTiming.cpp:8:0:
../../third_party/WebKit/Source/core/dom/Document.h:1395:69: error: type
attributes ignored after type is already defined [-Werror=attributes]
cc1plus: all warnings being treated as errors