Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(402)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 475803002: Make profiling lock global rather than per Target (Closed)

Created:
6 years, 4 months ago by yurys
Modified:
6 years, 4 months ago
Reviewers:
apavlov, sergeyv, pfeldman
CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org
Project:
blink
Visibility:
Public.

Description

Make profiling lock global rather than per Target We should pause any actions that could affect performance if any of the targets is being profiled, not only current target. BUG=403684 R=sergeyv@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180336

Patch Set 1 #

Total comments: 2

Patch Set 2 : Extracted Lock.js #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+83 lines, -113 lines) Patch
M Source/devtools/devtools.gypi View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/devtools/front_end/audits/AuditLauncherView.js View 3 chunks +4 lines, -4 lines 0 comments Download
A Source/devtools/front_end/common/Lock.js View 1 1 chunk +51 lines, -0 lines 1 comment Download
M Source/devtools/front_end/common/Object.js View 1 1 chunk +0 lines, -45 lines 0 comments Download
M Source/devtools/front_end/inspector.html View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/devtools/front_end/profiler/CanvasProfileView.js View 2 chunks +5 lines, -5 lines 0 comments Download
M Source/devtools/front_end/profiler/ProfilesPanel.js View 4 chunks +4 lines, -21 lines 0 comments Download
M Source/devtools/front_end/sdk/CSSStyleModel.js View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/devtools/front_end/sdk/DOMModel.js View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/devtools/front_end/sdk/DebuggerModel.js View 3 chunks +3 lines, -3 lines 0 comments Download
M Source/devtools/front_end/sdk/Target.js View 1 1 chunk +0 lines, -3 lines 0 comments Download
M Source/devtools/front_end/sdk/TimelineManager.js View 3 chunks +3 lines, -6 lines 0 comments Download
M Source/devtools/front_end/sdk/TracingModel.js View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/devtools/front_end/sdk/WorkerTargetManager.js View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/devtools/front_end/timeline/TimelinePanel.js View 4 chunks +2 lines, -18 lines 0 comments Download
M Source/devtools/scripts/frontend_modules.json View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
yurys
6 years, 4 months ago (2014-08-14 15:22:44 UTC) #1
sergeyv
https://codereview.chromium.org/475803002/diff/1/Source/devtools/front_end/sdk/Target.js File Source/devtools/front_end/sdk/Target.js (right): https://codereview.chromium.org/475803002/diff/1/Source/devtools/front_end/sdk/Target.js#newcode435 Source/devtools/front_end/sdk/Target.js:435: WebInspector.profilingLock = new WebInspector.Lock(); In my opinion it is ...
6 years, 4 months ago (2014-08-14 15:43:10 UTC) #2
yurys
https://codereview.chromium.org/475803002/diff/1/Source/devtools/front_end/sdk/Target.js File Source/devtools/front_end/sdk/Target.js (right): https://codereview.chromium.org/475803002/diff/1/Source/devtools/front_end/sdk/Target.js#newcode435 Source/devtools/front_end/sdk/Target.js:435: WebInspector.profilingLock = new WebInspector.Lock(); On 2014/08/14 15:43:10, sergeyv wrote: ...
6 years, 4 months ago (2014-08-14 15:54:11 UTC) #3
sergeyv
lgtm.
6 years, 4 months ago (2014-08-14 15:55:33 UTC) #4
yurys
The CQ bit was checked by yurys@chromium.org
6 years, 4 months ago (2014-08-14 15:59:20 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yurys@chromium.org/475803002/20001
6 years, 4 months ago (2014-08-14 16:00:11 UTC) #6
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_rel on tryserver.blink ...
6 years, 4 months ago (2014-08-14 17:14:57 UTC) #7
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-14 21:34:30 UTC) #8
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22821)
6 years, 4 months ago (2014-08-14 21:34:31 UTC) #9
yurys
The CQ bit was checked by yurys@chromium.org
6 years, 4 months ago (2014-08-15 07:32:21 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yurys@chromium.org/475803002/20001
6 years, 4 months ago (2014-08-15 07:33:28 UTC) #11
yurys
Committed patchset #2 manually as 180336 (presubmit successful).
6 years, 4 months ago (2014-08-15 07:38:02 UTC) #12
pfeldman
https://codereview.chromium.org/475803002/diff/20001/Source/devtools/front_end/common/Lock.js File Source/devtools/front_end/common/Lock.js (right): https://codereview.chromium.org/475803002/diff/20001/Source/devtools/front_end/common/Lock.js#newcode51 Source/devtools/front_end/common/Lock.js:51: WebInspector.profilingLock = new WebInspector.Lock(); Lets create this profiling lock ...
6 years, 4 months ago (2014-08-15 09:09:03 UTC) #13
pfeldman
6 years, 4 months ago (2014-08-15 09:14:25 UTC) #14
Message was sent while issue was closed. 
A revert of this CL (patchset #2) has been created in
https://codereview.chromium.org/471393002/ by pfeldman@chromium.org.

The reason for reverting is: 'common' should not be aware of profiler..

Powered by Google App Engine
This is Rietveld 408576698