Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2296923004:
Track frame context URL using first script heuristic (Closed)
|
Created:
4 years, 3 months ago by panicker Modified:
4 years, 3 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrack frame context URL using first script heuristic
BUG=635596
Committed: https://crrev.com/7012b38dafd3fcaaeaee7b7413718b4b6d7493ca
Cr-Commit-Position: refs/heads/master@{#418931}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Switch to Location ptr instead of constructing location URL string #
Total comments: 6
Patch Set 3 : Address review comments #
Total comments: 6
Patch Set 4 : Address review comments #
Total comments: 3
Patch Set 5 : add detailed comment & missing header #
Total comments: 4
Patch Set 6 : switch to tracking unique frame context location URLs from all script runs #
Total comments: 6
Patch Set 7 : address review comments #Patch Set 8 : rollforward with fix for unittest issue #
Total comments: 1
Messages
Total messages: 41 (14 generated)
panicker@chromium.org changed reviewers: + alph@chromium.org, caseq@chromium.org
PTAL. This CL adds a heuristic to capture Frame Context URL attribution. For background & preliminary data on the heuristic see this doc: https://docs.google.com/document/d/1J0brr10eHRpk0F6tRaWzkgI3mHja3SOHR6fwHBGzG... (The next CL after this one, will complete the plumbing by delivering the long task timing to Performance timing API.)
https://codereview.chromium.org/2296923004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2296923004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:43: std::string m_frameContextURL; you can't use std::string in core -- either use String (aka WTF::String) or, perhaps, just keep a reference to Location. It will let you get rid of m_firstScriptSeen as well.
On 2016/08/31 04:59:15, caseq wrote: > https://codereview.chromium.org/2296923004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): > > https://codereview.chromium.org/2296923004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:43: std::string > m_frameContextURL; > you can't use std::string in core -- either use String (aka WTF::String) or, > perhaps, just keep a reference to Location. It will let you get rid of > m_firstScriptSeen as well. Ah good to know. I'm OOO for the rest of this week. Will pick this up when I get back next week. Thanks!
https://codereview.chromium.org/2296923004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/BUILD.gn (right): https://codereview.chromium.org/2296923004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/BUILD.gn:1334: "inspector/InspectorWebPerfAgent.cpp", InspectorWebPerfAgentTest.cpp
PTAL. https://codereview.chromium.org/2296923004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/BUILD.gn (right): https://codereview.chromium.org/2296923004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/BUILD.gn:1334: "inspector/InspectorWebPerfAgent.cpp", On 2016/09/03 08:18:58, alph wrote: > InspectorWebPerfAgentTest.cpp Done. https://codereview.chromium.org/2296923004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2296923004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:43: std::string m_frameContextURL; On 2016/08/31 04:59:15, caseq wrote: > you can't use std::string in core -- either use String (aka WTF::String) or, > perhaps, just keep a reference to Location. It will let you get rid of > m_firstScriptSeen as well. Switched to keeping pointer to Location.
lgtm https://codereview.chromium.org/2296923004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2296923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:39: if (m_agent->m_frameContextLocation) { nit: early return if (!...) return ""; ... https://codereview.chromium.org/2296923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:50: return m_agent->m_frameContextLocation ? true : false; drop "? true : false"
https://codereview.chromium.org/2296923004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2296923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:37: std::string InspectorWebPerfAgentTest::frameContextURL() can we get rid of std::string here as well?
https://codereview.chromium.org/2296923004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2296923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:37: std::string InspectorWebPerfAgentTest::frameContextURL() On 2016/09/07 00:20:24, caseq wrote: > can we get rid of std::string here as well? Done. https://codereview.chromium.org/2296923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:39: if (m_agent->m_frameContextLocation) { On 2016/09/07 00:18:20, alph wrote: > nit: early return > if (!...) return ""; > ... Done. https://codereview.chromium.org/2296923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:50: return m_agent->m_frameContextLocation ? true : false; On 2016/09/07 00:18:20, alph wrote: > drop "? true : false" Done.
panicker@chromium.org changed reviewers: + pfeldman@chromium.org
+Pavel for BUILD.gn OWNERS
lgtm % comments https://codereview.chromium.org/2296923004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2296923004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:7: #include "core/InstrumentingAgents.h" do you need this? https://codereview.chromium.org/2296923004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:17: , m_frameContextLocation(nullptr) nit: no need for that, Member<> will do the job. https://codereview.chromium.org/2296923004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2296923004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:42: return String(location->protocol().ascii().data()) + "//" Now that this is not an std::string, ascii().data() is actually redundant (and would be harmful if it were to contain unicode).
https://codereview.chromium.org/2296923004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2296923004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:7: #include "core/InstrumentingAgents.h" On 2016/09/07 00:49:50, caseq wrote: > do you need this? not just yet https://codereview.chromium.org/2296923004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:17: , m_frameContextLocation(nullptr) On 2016/09/07 00:49:50, caseq wrote: > nit: no need for that, Member<> will do the job. Done. https://codereview.chromium.org/2296923004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2296923004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:42: return String(location->protocol().ascii().data()) + "//" On 2016/09/07 00:49:50, caseq wrote: > Now that this is not an std::string, ascii().data() is actually redundant (and > would be harmful if it were to contain unicode). oops, fixed.
The CQ bit was checked by panicker@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...
pfeldman@chromium.org changed reviewers: + dgozman@chromium.org
+dgozman for microtasks and entered contexts. https://codereview.chromium.org/2296923004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2296923004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:33: m_frameContextLocation = context->isDocument() ? toDocument(context)->location() : nullptr; May I ask how this will be used? There might be better ways of attributing that would not rely upon such heuristic: - storing location does not allow disambiguating ads served by the same provider - you are attributing extension costs to the frame - microtasks are all a part of a task, so all mutation observers and promises would be attributed to a wrong location We have ways to track entered v8 contexts (that are world-aware), so that you could save on storing one here. But the microtasks problem is fundamental, so we'd need to fix it on the v8 side.
https://codereview.chromium.org/2296923004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2296923004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:33: m_frameContextLocation = context->isDocument() ? toDocument(context)->location() : nullptr; On 2016/09/07 16:30:19, pfeldman wrote: > May I ask how this will be used? There might be better ways of attributing that > would not rely upon such heuristic: > > - storing location does not allow disambiguating ads served by the same provider > - you are attributing extension costs to the frame > - microtasks are all a part of a task, so all mutation observers and promises > would be attributed to a wrong location > > We have ways to track entered v8 contexts (that are world-aware), so that you > could save on storing one here. But the microtasks problem is fundamental, so > we'd need to fix it on the v8 side. The URL from this location will be surfaced as the name attribute in the Performance Entry. Explainer: https://github.com/spanicker/longtasks Doc: http://go/longtasks Yes this heuristic is not perfect, but it works for common cases based on preliminary data and we'll keep refining this incrementally. I've summarized the initial data here: https://docs.google.com/document/d/1H9ZsRh5G3RErYRtzHzR0Sl_5n2clMeKqIdg91Atpb... Note that this is the V1 API - here we want to indicate the iframe's Location URL, to give an indication of which iframes are causing problems for the site. For V2 we want to show additional information such as the script URL (which would help with disambiguating Ads case for instance) or implicate a function in the call stack if possible. The discussion on what to surface for V2 is ongoing, so your input there is very appreciated. Yes extensions and microtasks are tricky to attribute, and I haven't found a good way to attribute those yet. Ideas there are very welcome. I'll start a separate thread for these discussions. The overall idea here is to start with something simple and imperfect and keep adding additional attribution incrementally. Even the basic V1 is going to be useful for sites with 3rd party content & Ads.
Microtasks from different pages running in the same context should not mix, since they are always executed at the end of the task, and different pages have different top-level tasks. But if one frame triggers, say, mutation observer set in another frame, then all microtasks would be attributed to the enclosing task. I'm not even sure how we should attribute those - to the first frame or the second one?
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_...)
PTAL. Updated per our discussion on Friday. Any other concerns here? https://codereview.chromium.org/2296923004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2296923004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:33: m_frameContextLocation = context->isDocument() ? toDocument(context)->location() : nullptr; On 2016/09/07 16:52:02, Shubhie wrote: > On 2016/09/07 16:30:19, pfeldman wrote: > > May I ask how this will be used? There might be better ways of attributing > that > > would not rely upon such heuristic: > > > > - storing location does not allow disambiguating ads served by the same > provider > > - you are attributing extension costs to the frame > > - microtasks are all a part of a task, so all mutation observers and promises > > would be attributed to a wrong location > > > > We have ways to track entered v8 contexts (that are world-aware), so that you > > could save on storing one here. But the microtasks problem is fundamental, so > > we'd need to fix it on the v8 side. > > The URL from this location will be surfaced as the name attribute in the > Performance Entry. > Explainer: https://github.com/spanicker/longtasks > Doc: http://go/longtasks > > Yes this heuristic is not perfect, but it works for common cases based on > preliminary data and we'll keep refining this incrementally. > I've summarized the initial data here: > https://docs.google.com/document/d/1H9ZsRh5G3RErYRtzHzR0Sl_5n2clMeKqIdg91Atpb... > > Note that this is the V1 API - here we want to indicate the iframe's Location > URL, to give an indication of which iframes are causing problems for the site. > For V2 we want to show additional information such as the script URL (which > would help with disambiguating Ads case for instance) or implicate a function in > the call stack if possible. > The discussion on what to surface for V2 is ongoing, so your input there is very > appreciated. > Yes extensions and microtasks are tricky to attribute, and I haven't found a > good way to attribute those yet. Ideas there are very welcome. > I'll start a separate thread for these discussions. > > The overall idea here is to start with something simple and imperfect and keep > adding additional attribution incrementally. Even the basic V1 is going to be > useful for sites with 3rd party content & Ads. > Resolved per our discussion last week: - added more detailed comment in CL with link to doc: go/longtasks-v2 [WIP] - I've added an "Alternatives considered" section to the go/longtasks to address the question of why the API provides toplevel task timing vs subtasks only. See: https://docs.google.com/document/d/1uTBY7wWIWpgKF4EXSu6B-95JxtqDZP0jD42neygWA... - I've added summary of our Friday discussion here: https://docs.google.com/document/d/1T2LmEkI294PEy1hFqK0aehoz6dA_TKRZEzBeIKQjW...
https://codereview.chromium.org/2296923004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2296923004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:36: // See http://go/longtasks-v2 We don't usually commit internal Google links to open-source Chromium. https://codereview.chromium.org/2296923004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:39: m_frameContextLocation = context->isDocument() ? toDocument(context)->location() : nullptr; As discussed, let's either: - collect all different locations to report long tasks to all parties involved; or - ignore the task that had multiple locations inside. This way we won't blame some random page if there were multiple.
PTAL https://codereview.chromium.org/2296923004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2296923004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:36: // See http://go/longtasks-v2 On 2016/09/12 18:41:39, dgozman wrote: > We don't usually commit internal Google links to open-source Chromium. Okay will add link later when it's ready for external consumption. https://codereview.chromium.org/2296923004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:39: m_frameContextLocation = context->isDocument() ? toDocument(context)->location() : nullptr; On 2016/09/12 18:41:39, dgozman wrote: > As discussed, let's either: > - collect all different locations to report long tasks to all parties involved; > or > - ignore the task that had multiple locations inside. > > This way we won't blame some random page if there were multiple. Good point, switched to tracking frame contexts from all scripts runs.
lgtm https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:48: // Reset m_frameContextURL. We don't reset this in didProcessTask as Comment says |m_frameContextURL| which does not exist anymore. https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:44: HeapHashSet<Member<Location>> m_frameContextLocation; nit: m_frameContextLocations https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:67: TEST_F(InspectorWebPerfAgentTest, MultipleScriptsInTask) Let's have a test for two contexts with different locations.
PTAL. (Sorry for the delay - was bit swamped with Perf. Phew, glad it's over :)) https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp (right): https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.cpp:48: // Reset m_frameContextURL. We don't reset this in didProcessTask as On 2016/09/12 21:12:04, dgozman wrote: > Comment says |m_frameContextURL| which does not exist anymore. Done. https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h (right): https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgent.h:44: HeapHashSet<Member<Location>> m_frameContextLocation; On 2016/09/12 21:12:04, dgozman wrote: > nit: m_frameContextLocations Done. https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2296923004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:67: TEST_F(InspectorWebPerfAgentTest, MultipleScriptsInTask) On 2016/09/12 21:12:04, dgozman wrote: > Let's have a test for two contexts with different locations. Done.
Friendly ping owners approval.
Whoops, looks like I have the approvals. Pavel, any other comments? otherwise I will submit this today. Thanks all!
The CQ bit was checked by panicker@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by panicker@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org, caseq@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2296923004/#ps120001 (title: "address review comments")
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 #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Track frame context URL using first script heuristic BUG=635596 ========== to ========== Track frame context URL using first script heuristic BUG=635596 Committed: https://crrev.com/7012b38dafd3fcaaeaee7b7413718b4b6d7493ca Cr-Commit-Position: refs/heads/master@{#418931} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7012b38dafd3fcaaeaee7b7413718b4b6d7493ca Cr-Commit-Position: refs/heads/master@{#418931}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2345023002/ by msw@chromium.org. The reason for reverting is: broke compile: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Bu... c:\b\c\b\win_layout\src\third_party\webkit\source\core\inspector\inspectorwebperfagenttest.cpp(48) : error C2220: warning treated as error - no 'object' file generated c:\b\c\b\win_layout\src\third_party\webkit\source\core\inspector\inspectorwebperfagenttest.cpp(48) : warning C4702: unreachable code .
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 418931 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
https://codereview.chromium.org/2296923004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp (right): https://codereview.chromium.org/2296923004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorWebPerfAgentTest.cpp:48: Location* location = (*m_agent->m_frameContextLocations.begin()).get(); This is the only line that changed in the fix with rollforward |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
