New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 618221 link

Starred by 1 user
Status: Fixed
Owner:
ksakamoto@chromium.org
Closed: Jun 2016
Cc:
xiaoche...@chromium.org
kouhei@chromium.org
benjhayden@chromium.org
skyos...@chromium.org
caseq@chromium.org
paulir...@chromium.org
skare@chromium.org
isherman@chromium.org
asvitk...@chromium.org
█ rkaplow@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Frame IDs in trace events are not logged in a consistent format

Project Member Reported by ksakamoto@chromium.org, Jun 8 2016 
FirstMeaningfulPaint metric is computed from several types of trace events. These events are associated using Frame ID, which is LocalFrame pointer value.

However, this value is not stringified in a consistent way. In 32-bit platforms they look like this:

navigationStart: "args":{"frame":"0xffffffffa0dc1a38"}
FrameLoader snapshot: {"id_ref":"0xa0dc1a38"}
Paint: {"frame":"0xffffffffa0dc1a38"}
FrameView::performLayout: {"frame":"0xa0dc1a38"}

For navigationStart and Paint, the pointer value is casted to intptr_t and then uint64_t:
https://cs.chromium.org/chromium/src/base/trace_event/trace_event_impl.cc?l=259

The others use uintptr_t, so sign extention does not happen.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp?rcl=0&l=1619
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/FrameView.cpp?rcl=0&l=855

Comment 1 by benjhayden@chromium.org, Jun 8 2016

Cc: skyos...@chromium.org caseq@chromium.org xiaoche...@chromium.org
Grouping ThreadSlices by their Frame should eventually be accomplished using a general context-annotation system such as FrameBlamer, instead of tracing bespoke LocalFrame pointers in a fragile way.
IIUC, the FrameBlamer way of grouping object snapshots together with their frame is to use an id_ref to the FrameTreeNode or RenderFrame BlameContext. I'm not sure how to refer to a FrameBlameContext from inside blink. skyostil, xiaochengh, ideas?

I started using FrameBlameContexts to group RenderFrame, paint, and layout events in this CL, but it's on hold while xiaochengh improves FrameBlamer towards the point where we won't need explicit LocalFrame pointers on specific trace events.
https://codereview.chromium.org/2020813002

That is all just FYI.
In the mean time, I think that navigationStart and devtools' Paint events should use uintptr_t. Does that sound right to everybody?

+caseq for devtools

+skyostil,xiaochengh in case they have relevant updates on FrameBlamer; FYI if not.

AFAIK, nobody keeps a close ownership over navigationStart, so anybody can fix that, unless somebody knows if the owner should be consulted.

Comment 2 by ksakamoto@chromium.org, Jun 9 2016 

Thanks Ben for the information.

> In the mean time, I think that navigationStart and devtools' Paint events should use uintptr_t. Does that sound right to everybody?

That sounds good to me.
Created a patch that replaces intptr_t with uintptr_t in pointer stringification: https://codereview.chromium.org/2043233003/

If this turns out to be troublesome, we can change the other two to match navigationStart/paint.

Comment 3 by xiaoche...@chromium.org, Jun 9 2016 

> I'm not sure how to refer to a FrameBlameContext from inside blink.

Given a LocalFrame* named |frame|, the corresponding FrameBlameContext can be obtained as:

frame->client()->frameBlameContext()

Both client() and frameBlameContext() may return nullptr and need to be checked.
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 11 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e853dea3b3213b3736f0fa2f36aa8ae05950edf9

commit e853dea3b3213b3736f0fa2f36aa8ae05950edf9
Author: ksakamoto <ksakamoto@chromium.org>
Date: Sat Jun 11 01:29:47 2016

Prevent sign extension for pointer trace values

This makes 32-bit pointers serialize like "0xdeadbeef", not
"0xffffffffdeadbeef".

BUG= 618221 

Review-Url: https://codereview.chromium.org/2043233003
Cr-Commit-Position: refs/heads/master@{#399347}

[modify] https://crrev.com/e853dea3b3213b3736f0fa2f36aa8ae05950edf9/base/trace_event/trace_event_impl.cc
[modify] https://crrev.com/e853dea3b3213b3736f0fa2f36aa8ae05950edf9/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e853dea3b3213b3736f0fa2f36aa8ae05950edf9

commit e853dea3b3213b3736f0fa2f36aa8ae05950edf9
Author: ksakamoto <ksakamoto@chromium.org>
Date: Sat Jun 11 01:29:47 2016

Prevent sign extension for pointer trace values

This makes 32-bit pointers serialize like "0xdeadbeef", not
"0xffffffffdeadbeef".

BUG= 618221 

Review-Url: https://codereview.chromium.org/2043233003
Cr-Commit-Position: refs/heads/master@{#399347}

[modify] https://crrev.com/e853dea3b3213b3736f0fa2f36aa8ae05950edf9/base/trace_event/trace_event_impl.cc
[modify] https://crrev.com/e853dea3b3213b3736f0fa2f36aa8ae05950edf9/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp

Comment 6 by ksakamoto@chromium.org, Jun 21 2016

Status: Fixed (was: Assigned)
►

Sign in to add a comment