Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
106353005:
Expose performance.memory in workers (Closed)
|
Created:
6 years, 12 months ago by Pan Modified:
5 years, 12 months ago CC:
blink-reviews, RyanS, komoroske Base URL:
https://chromium.googlesource.com/chromium/blink.git@Perf-Memory-SharedWorker Visibility:
Public. |
DescriptionExpose performance.memory in workers, runtimeEnabledFeatures is used to control if real or quantized heap size info will be returned.
BUG=326370
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173450
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 5
Patch Set 3 : Introduce WorkerSettings #
Total comments: 12
Patch Set 4 : #
Total comments: 13
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 9
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 6
Patch Set 9 : #
Total comments: 1
Messages
Total messages: 47 (1 generated)
please review :) thanks Pan
please review :) thanks Pan
I'm not a great reviewer for this change and simonjam is working on another project now. Perhaps abarth or adamk might be willing to review?
https://codereview.chromium.org/106353005/diff/1/Source/core/workers/Dedicate... File Source/core/workers/DedicatedWorkerGlobalScope.h (right): https://codereview.chromium.org/106353005/diff/1/Source/core/workers/Dedicate... Source/core/workers/DedicatedWorkerGlobalScope.h:46: static PassRefPtr<DedicatedWorkerGlobalScope> create(DedicatedWorkerThread*, PassOwnPtr<WorkerThreadStartupData>, double timeOrigin, bool memoryInfoEnabled); It's not correct to pass memoryInfoEnabled here. We need to find a better way to read settings on workers. Do we have a WorkerSettings object? Should we use RuntimeEnabledFeatures?
https://codereview.chromium.org/106353005/diff/1/Source/core/workers/Dedicate... File Source/core/workers/DedicatedWorkerGlobalScope.h (right): https://codereview.chromium.org/106353005/diff/1/Source/core/workers/Dedicate... Source/core/workers/DedicatedWorkerGlobalScope.h:46: static PassRefPtr<DedicatedWorkerGlobalScope> create(DedicatedWorkerThread*, PassOwnPtr<WorkerThreadStartupData>, double timeOrigin, bool memoryInfoEnabled); On 2014/01/07 06:59:16, abarth wrote: > It's not correct to pass memoryInfoEnabled here. We need to find a better way > to read settings on workers. Do we have a WorkerSettings object? Should we use > RuntimeEnabledFeatures? Currently, workers don't have a WorkerSettings object. we can create one sit in WorkerGlobalScope, memoryEnabled and other settings that needed could be there. or do you mean dedicated worker should read settings of its document every time in case it is changed?
On 2014/01/07 09:30:58, Pan wrote: > https://codereview.chromium.org/106353005/diff/1/Source/core/workers/Dedicate... > File Source/core/workers/DedicatedWorkerGlobalScope.h (right): > > https://codereview.chromium.org/106353005/diff/1/Source/core/workers/Dedicate... > Source/core/workers/DedicatedWorkerGlobalScope.h:46: static > PassRefPtr<DedicatedWorkerGlobalScope> create(DedicatedWorkerThread*, > PassOwnPtr<WorkerThreadStartupData>, double timeOrigin, bool memoryInfoEnabled); > On 2014/01/07 06:59:16, abarth wrote: > > It's not correct to pass memoryInfoEnabled here. We need to find a better way > > to read settings on workers. Do we have a WorkerSettings object? Should we > use > > RuntimeEnabledFeatures? > > Currently, workers don't have a WorkerSettings object. we can create one sit in > WorkerGlobalScope, memoryEnabled and other settings that needed could be there. > or do you mean dedicated worker should read settings of its document every time > in case it is changed? I meant the former. I see that you've got adamk listed as a reviewer on this CL. Hopefully he'll be able to take a look. I'm sorry, but I don't have the bandwidth to review this CL.
thanks @abarth, Hi @adamk, To make dedicated worker aware the settings (which is from chrome switch. --enable-memory-info) can be straightforward, just copy part of settings from its parent doc. To make settings in shared worker, seems we have to change chrome part code, either through IPC from render to tell settings(similar to dedicated worker), or pass *chrome::switches* when fork a worker process. personally, I prefer the later one. how do you think of it? thanks Pan
On 2014/02/26 10:39:58, Pan wrote: > thanks @abarth, > > Hi @adamk, > > To make dedicated worker aware the settings (which is from chrome switch. > --enable-memory-info) can be straightforward, just copy part of settings from > its parent doc. > To make settings in shared worker, seems we have to change chrome part code, > either through IPC from render to tell settings(similar to dedicated worker), or > pass *chrome::switches* when fork a worker process. personally, I prefer the > later one. how do you think of it? > > thanks > Pan Seems reasonable to pass the appropriate switches to the shared worker process; kinuko has been doing a lot of work in this area and would probably be the right person to work with on that.
> Seems reasonable to pass the appropriate switches to the shared worker process; > kinuko has been doing a lot of work in this area and would probably be the right > person to work with on that. thanks, @adamk Hi @kinuko, 2nd patchset shows the general idea, it introduces WorkerSettings to WorkGlobalScope, For, dedicated worker, WorkerSettings comes from its parent doc. Shared worker, comes from embed switch query at client side, related chrome side change @https://codereview.chromium.org/183153002 Service worker, default WorkerSettings is used, which means not good granular memory info in it. please review, thanks! Pan
Could we split this code into two, one for WorkerSettings plumbing and the other for enabling memoryInfo? Also in this way the settings won't be able to be changed once it's passed to WorkerGlobalScope, and it sounds like RuntimEnabledFeature might be enough? (from the comments in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) https://codereview.chromium.org/106353005/diff/250001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/106353005/diff/250001/Source/core/workers/Wor... Source/core/workers/WorkerThread.h:104: OwnPtr<WorkerSettings> m_settings; Do we need to keep owning WorkerSettings here? Can't this just passed to the WorkerGlobalScope when it's created, and if so can it be included in WorkerThreadStartupData? https://codereview.chromium.org/106353005/diff/250001/public/web/WebSharedWor... File public/web/WebSharedWorkerClient.h (right): https://codereview.chromium.org/106353005/diff/250001/public/web/WebSharedWor... public/web/WebSharedWorkerClient.h:56: virtual bool enableMemoryInfo() { return false; } I'd like to avoid adding specific settings/preference method here. I'm not very familiar with the settings code but, say, could we reuse WebSettings interface or define new WebWorkerSettings to expose it to chromium?
https://codereview.chromium.org/106353005/diff/250001/public/web/WebSharedWor... File public/web/WebSharedWorkerClient.h (right): https://codereview.chromium.org/106353005/diff/250001/public/web/WebSharedWor... public/web/WebSharedWorkerClient.h:56: virtual bool enableMemoryInfo() { return false; } On 2014/02/28 05:23:29, kinuko wrote: > I'd like to avoid adding specific settings/preference method here. > > I'm not very familiar with the settings code but, say, could we reuse > WebSettings interface or define new WebWorkerSettings to expose it to chromium? I guess you mean we need a general method something like: virtual WebSettings(WebWorkerSettings) workerSettings() { xxxx }; if that is true, either we need a new WebWorkerSettings in web platform API, or we need change WebSettings, which is an pure abstract class currently. I think the 1st solution is reasonable. your idea? thanks Pan
> https://codereview.chromium.org/106353005/diff/250001/public/web/WebSharedWor... > public/web/WebSharedWorkerClient.h:56: virtual bool enableMemoryInfo() { return > false; } > On 2014/02/28 05:23:29, kinuko wrote: > > I'd like to avoid adding specific settings/preference method here. > > > > I'm not very familiar with the settings code but, say, could we reuse > > WebSettings interface or define new WebWorkerSettings to expose it to > chromium? > > I guess you mean we need a general method something like: > virtual WebSettings(WebWorkerSettings) workerSettings() { xxxx }; > > if that is true, either we need a new WebWorkerSettings in web platform API, or > we need change WebSettings, which is an pure abstract class currently. I think > the 1st solution is reasonable. Yes, I'm asking if we could have a more generic mechanism for worker settings. For documents we expose WebView->settings() from blink to chromium, and probably we could think similar mechanism for workers too. WebSettings has way too many settings that may not be interested in workers so having WebWorkerSettings sounds reasonable to me. Also before you start working on WorkerSettings, how do you think about my comments below: On 2014/02/28 05:23:28, kinuko wrote: > Could we split this code into two, one for WorkerSettings plumbing and the > other for enabling memoryInfo? > > Also in this way the settings won't be able to be changed once it's passed to > WorkerGlobalScope, and it sounds like RuntimEnabledFeature might be enough? > (from the comments in > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
On 2014/02/28 10:23:42, kinuko wrote: > > > https://codereview.chromium.org/106353005/diff/250001/public/web/WebSharedWor... > > public/web/WebSharedWorkerClient.h:56: virtual bool enableMemoryInfo() { > return > > false; } > > On 2014/02/28 05:23:29, kinuko wrote: > > > I'd like to avoid adding specific settings/preference method here. > > > > > > I'm not very familiar with the settings code but, say, could we reuse > > > WebSettings interface or define new WebWorkerSettings to expose it to > > chromium? > > > > I guess you mean we need a general method something like: > > virtual WebSettings(WebWorkerSettings) workerSettings() { xxxx }; > > > > if that is true, either we need a new WebWorkerSettings in web platform API, > or > > we need change WebSettings, which is an pure abstract class currently. I think > > the 1st solution is reasonable. > > Yes, I'm asking if we could have a more generic mechanism for worker settings. > For documents we expose WebView->settings() from blink to chromium, and probably > we could think similar mechanism for workers too. WebSettings has way too many > settings that may not be interested in workers so having WebWorkerSettings > sounds reasonable to me. > > Also before you start working on WorkerSettings, how do you think about my > comments below: > > On 2014/02/28 05:23:28, kinuko wrote: > > Could we split this code into two, one for WorkerSettings plumbing and the > > other for enabling memoryInfo? > > > > Also in this way the settings won't be able to be changed once it's passed to > > WorkerGlobalScope, and it sounds like RuntimEnabledFeature might be enough? > > (from the comments in > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) of course, quite agree with that, thanks. :) Back to plumb WorkerSettings for shared worker, the mechanism in patchset 2 is embeding chromium's setting to blink. I think you prefer to expose WebWorkerSettings to chromium, and make chromium set the settings for sharedworker, right? I've struggled with these two choices, the "embed" was adopted in patchset2 is because I was afraid of touch web platform api too much :) thanks Pan
Hi kinuko, ready to review, thanks! Pan https://codereview.chromium.org/106353005/diff/250001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/106353005/diff/250001/Source/core/workers/Wor... Source/core/workers/WorkerThread.h:104: OwnPtr<WorkerSettings> m_settings; On 2014/02/28 05:23:29, kinuko wrote: > Do we need to keep owning WorkerSettings here? Can't this just passed to the > WorkerGlobalScope when it's created, and if so can it be included in > WorkerThreadStartupData? done, thanks https://codereview.chromium.org/106353005/diff/250001/public/web/WebSharedWor... File public/web/WebSharedWorkerClient.h (right): https://codereview.chromium.org/106353005/diff/250001/public/web/WebSharedWor... public/web/WebSharedWorkerClient.h:56: virtual bool enableMemoryInfo() { return false; } > I guess you mean we need a general method something like: > virtual WebSettings(WebWorkerSettings) workerSettings() { xxxx }; > > if that is true, either we need a new WebWorkerSettings in web platform API, or > we need change WebSettings, which is an pure abstract class currently. I think > the 1st solution is reasonable. > your idea? > > thanks > Pan Pachset 3 exposes WebWorkerSettings through WebAPI, can be set by chromium, see another thread https://codereview.chromium.org/183153002/.
https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... File Source/core/workers/WorkerGlobalScope.h (right): https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... Source/core/workers/WorkerGlobalScope.h:147: WorkerGlobalScope(const KURL&, const String& userAgent, WorkerThread*, double timeOrigin, PassOwnPtr<WorkerClients>, PassOwnPtr<WorkerSettings>); nit: extra space after timeOrigin, https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... File Source/core/workers/WorkerMessagingProxy.cpp (right): https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... Source/core/workers/WorkerMessagingProxy.cpp:117: settings->setMemoryInfoEnabled(document->settings()->memoryInfoEnabled()); Could we have these settings-to-settings copy methods in a more centralized way? Say, could we write WorkerSettings::create(document->settings()) to copy all relevant settings from documents to worker's one? Then we don't need to edit many files just to add new settings fields. https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... File Source/core/workers/WorkerSettings.h (right): https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... Source/core/workers/WorkerSettings.h:2: * Copyright (C) 2014 Intel Corporation. All rights reserved. Can we use shorter common copyright for new files? http://www.chromium.org/blink/coding-style#TOC-License https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... Source/core/workers/WorkerSettings.h:40: WTF_MAKE_NONCOPYABLE(WorkerSettings); WTF_MAKE_FAST_ALLOCATED; nit: looks like this could be copyable unless we want to explicitly disallow https://codereview.chromium.org/106353005/diff/270001/Source/web/WebSharedWor... File Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/106353005/diff/270001/Source/web/WebSharedWor... Source/web/WebSharedWorkerImpl.cpp:355: settings->setMemoryInfoEnabled(m_settings->memoryInfoEnabled()); As I commented in the other file I prefer having these methods in a centralized place. Maybe WebWorkerSettings could have an operator for automatic conversion to WorkerSettings (exposed only if BLINK_IMPLEMENTATION is defined)? https://codereview.chromium.org/106353005/diff/270001/Source/web/WebSharedWor... File Source/web/WebSharedWorkerImpl.h (right): https://codereview.chromium.org/106353005/diff/270001/Source/web/WebSharedWor... Source/web/WebSharedWorkerImpl.h:109: virtual WebWorkerSettings* settings() OVERRIDE { return m_settings.get(); } Unlike WebView case changing this after calling startWorkerContext() doesn't have any effects, and I think it should be just passed to startWorkerContext(). Also WebWorkerSettings could probably be a very simple value class.
Hi @kinuko, please review. thanks Pan https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... File Source/core/workers/WorkerGlobalScope.h (right): https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... Source/core/workers/WorkerGlobalScope.h:147: WorkerGlobalScope(const KURL&, const String& userAgent, WorkerThread*, double timeOrigin, PassOwnPtr<WorkerClients>, PassOwnPtr<WorkerSettings>); On 2014/03/04 04:34:35, kinuko wrote: > nit: extra space after timeOrigin, Done. https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... File Source/core/workers/WorkerMessagingProxy.cpp (right): https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... Source/core/workers/WorkerMessagingProxy.cpp:117: settings->setMemoryInfoEnabled(document->settings()->memoryInfoEnabled()); On 2014/03/04 04:34:35, kinuko wrote: > Could we have these settings-to-settings copy methods in a more centralized way? > Say, could we write WorkerSettings::create(document->settings()) to copy all > relevant settings from documents to worker's one? Then we don't need to edit > many files just to add new settings fields. right, thanks！ Done. https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... File Source/core/workers/WorkerSettings.h (right): https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... Source/core/workers/WorkerSettings.h:2: * Copyright (C) 2014 Intel Corporation. All rights reserved. On 2014/03/04 04:34:35, kinuko wrote: > Can we use shorter common copyright for new files? > > http://www.chromium.org/blink/coding-style#TOC-License Done. https://codereview.chromium.org/106353005/diff/270001/Source/core/workers/Wor... Source/core/workers/WorkerSettings.h:40: WTF_MAKE_NONCOPYABLE(WorkerSettings); WTF_MAKE_FAST_ALLOCATED; On 2014/03/04 04:34:35, kinuko wrote: > nit: looks like this could be copyable unless we want to explicitly disallow Done. https://codereview.chromium.org/106353005/diff/270001/Source/web/WebSharedWor... File Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/106353005/diff/270001/Source/web/WebSharedWor... Source/web/WebSharedWorkerImpl.cpp:355: settings->setMemoryInfoEnabled(m_settings->memoryInfoEnabled()); On 2014/03/04 04:34:35, kinuko wrote: > As I commented in the other file I prefer having these methods in a centralized > place. Maybe WebWorkerSettings could have an operator for automatic conversion > to WorkerSettings (exposed only if BLINK_IMPLEMENTATION is defined)? thanks! done https://codereview.chromium.org/106353005/diff/270001/Source/web/WebSharedWor... File Source/web/WebSharedWorkerImpl.h (right): https://codereview.chromium.org/106353005/diff/270001/Source/web/WebSharedWor... Source/web/WebSharedWorkerImpl.h:109: virtual WebWorkerSettings* settings() OVERRIDE { return m_settings.get(); } On 2014/03/04 04:34:35, kinuko wrote: > Unlike WebView case changing this after calling startWorkerContext() doesn't > have any effects, and I think it should be just passed to startWorkerContext(). > Also WebWorkerSettings could probably be a very simple value class. yes, remove this impl
lgtm modulo some nits. (It'd be better to start asking API owners to review public API changes) https://codereview.chromium.org/106353005/diff/330001/Source/core/workers/Sha... File Source/core/workers/SharedWorkerGlobalScope.h (right): https://codereview.chromium.org/106353005/diff/330001/Source/core/workers/Sha... Source/core/workers/SharedWorkerGlobalScope.h:61: SharedWorkerGlobalScope(const String& name, const KURL&, const String& userAgent, SharedWorkerThread*, PassOwnPtr<WorkerClients>, PassOwnPtr<WorkerSettings>); nit: extra space after SharedWorkerThread*, https://codereview.chromium.org/106353005/diff/330001/Source/core/workers/Wor... File Source/core/workers/WorkerSettings.h (right): https://codereview.chromium.org/106353005/diff/330001/Source/core/workers/Wor... Source/core/workers/WorkerSettings.h:41: class WorkerSettings { Can we have a class comment for this? https://codereview.chromium.org/106353005/diff/330001/Source/core/workers/Wor... Source/core/workers/WorkerSettings.h:52: WorkerSettings(Settings*); explicit https://codereview.chromium.org/106353005/diff/330001/Source/web/WebSharedWor... File Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/106353005/diff/330001/Source/web/WebSharedWor... Source/web/WebSharedWorkerImpl.cpp:159: , m_settings(WebWorkerSettings::create()) not needed https://codereview.chromium.org/106353005/diff/330001/Source/web/WebWorkerSet... File Source/web/WebWorkerSettings.cpp (right): https://codereview.chromium.org/106353005/diff/330001/Source/web/WebWorkerSet... Source/web/WebWorkerSettings.cpp:43: settings->setMemoryInfoEnabled(m_private->memoryInfoEnabled()); m_private itself is WorkerSettings, for our usage something like following looks enough to me PassOwnPtr<WorkerSettings> release() { return m_private.release(); } https://codereview.chromium.org/106353005/diff/330001/public/web/WebWorkerSet... File public/web/WebWorkerSettings.h (right): https://codereview.chromium.org/106353005/diff/330001/public/web/WebWorkerSet... public/web/WebWorkerSettings.h:22: // modify the settings for the WebSharedWorker. This comment looks wrong now?
https://codereview.chromium.org/106353005/diff/330001/Source/web/WebWorkerSet... File Source/web/WebWorkerSettings.cpp (right): https://codereview.chromium.org/106353005/diff/330001/Source/web/WebWorkerSet... Source/web/WebWorkerSettings.cpp:43: settings->setMemoryInfoEnabled(m_private->memoryInfoEnabled()); On 2014/03/10 05:55:52, kinuko wrote: > m_private itself is WorkerSettings, for our usage something like following looks > enough to me > > PassOwnPtr<WorkerSettings> release() { return m_private.release(); } this method is also used in assign(const WebWorkerSettings&), from copy constructor and operator=. otherwise I need to add a create(WorkerSettings*) to WebCore::WorkerSettings then use it in assign, meanwhile, in WebWorkerSettings.h, add a '#include "core/workers/WorkerSettings.h"' between BLINK_IMPLEMENTATION. I don't have strong preference, is the later one acceptable? thanks Pan
https://codereview.chromium.org/106353005/diff/330001/Source/core/workers/Wor... File Source/core/workers/WorkerSettings.h (right): https://codereview.chromium.org/106353005/diff/330001/Source/core/workers/Wor... Source/core/workers/WorkerSettings.h:2: * Copyright (C) 2014 Intel Corporation. All rights reserved. Can we use the common chromium copyright for newer files? (You're listed in chromium authors) http://www.chromium.org/blink/coding-style#TOC-License https://codereview.chromium.org/106353005/diff/330001/Source/web/WebSharedWor... File Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/106353005/diff/330001/Source/web/WebSharedWor... Source/web/WebSharedWorkerImpl.cpp:355: OwnPtr<WorkerThreadStartupData> startupData = WorkerThreadStartupData::create(m_url, m_loadingDocument->userAgent(m_url), m_mainScriptLoader->script(), startMode, m_contentSecurityPolicy, static_cast<WebCore::ContentSecurityPolicy::HeaderType>(m_policyType), workerClients.release(), PassOwnPtr<WorkerSettings>(m_settings)); I don't think we need this explicit cast (just passing m_settings should work) https://codereview.chromium.org/106353005/diff/330001/Source/web/WebWorkerSet... File Source/web/WebWorkerSettings.cpp (right): https://codereview.chromium.org/106353005/diff/330001/Source/web/WebWorkerSet... Source/web/WebWorkerSettings.cpp:43: settings->setMemoryInfoEnabled(m_private->memoryInfoEnabled()); On 2014/03/10 12:27:52, Pan wrote: > On 2014/03/10 05:55:52, kinuko wrote: > > m_private itself is WorkerSettings, for our usage something like following > looks > > enough to me > > > > PassOwnPtr<WorkerSettings> release() { return m_private.release(); } > > this method is also used in assign(const WebWorkerSettings&), from copy > constructor and operator=. > otherwise I need to add a create(WorkerSettings*) to WebCore::WorkerSettings > then use it in assign, meanwhile, in WebWorkerSettings.h, add a '#include > "core/workers/WorkerSettings.h"' between BLINK_IMPLEMENTATION. > I don't have strong preference, is the later one acceptable? I see, I overlooked it. I'd add WorkerSettings::create(const WorkerSettings&) (which can just call new WorkerSettings(settings) as it's copyable) then, and use it in assign() and in this operator (with *m_private.get()). I don't see why we need to include WorkerSettings.h in WebWorkerSettings.h though, it doesn't seem necessary?
https://codereview.chromium.org/106353005/diff/330001/public/web/WebSharedWor... File public/web/WebSharedWorker.h (right): https://codereview.chromium.org/106353005/diff/330001/public/web/WebSharedWor... public/web/WebSharedWorker.h:60: const WebWorkerSettings& = WebWorkerSettings::create()) = 0; This function is growing many arguments. Maybe we should create a struct for these values? https://codereview.chromium.org/106353005/diff/330001/public/web/WebWorkerSet... File public/web/WebWorkerSettings.h (right): https://codereview.chromium.org/106353005/diff/330001/public/web/WebWorkerSet... public/web/WebWorkerSettings.h:39: BLINK_EXPORT void setMemoryInfoEnabled(bool); Does this setting need to be per-worker? Do we not want to use a per-process setting?
https://codereview.chromium.org/106353005/diff/330001/public/web/WebWorkerSet... File public/web/WebWorkerSettings.h (right): https://codereview.chromium.org/106353005/diff/330001/public/web/WebWorkerSet... public/web/WebWorkerSettings.h:39: BLINK_EXPORT void setMemoryInfoEnabled(bool); If this maps to a command-line flag in Chromium, it sounds like this would be better as a per-process runtime-enabled feature.
On 2014/03/10 21:33:28, abarth wrote: > https://codereview.chromium.org/106353005/diff/330001/public/web/WebWorkerSet... > File public/web/WebWorkerSettings.h (right): > > https://codereview.chromium.org/106353005/diff/330001/public/web/WebWorkerSet... > public/web/WebWorkerSettings.h:39: BLINK_EXPORT void setMemoryInfoEnabled(bool); > If this maps to a command-line flag in Chromium, it sounds like this would be > better as a per-process runtime-enabled feature. thanks @abarth, @kinuko, I don't object to use runtime enabled feature, things will be much easier as we will touch less workers' code. while we need two runtime feature flags, a MemoryInfoInWorkers to guard this feature, and another PreciseMemoryInfo to control memory info granularity. please review. thanks Pan
https://codereview.chromium.org/106353005/diff/350001/LayoutTests/http/tests/... File LayoutTests/http/tests/w3c/webperf/resources/worker.js (right): https://codereview.chromium.org/106353005/diff/350001/LayoutTests/http/tests/... LayoutTests/http/tests/w3c/webperf/resources/worker.js:16: postMessage(JSON.stringify(results)); why do we need to indirect via JSON and not rely on structured cloning? https://codereview.chromium.org/106353005/diff/350001/Source/modules/performa... File Source/modules/performance/WorkerPerformance.h (right): https://codereview.chromium.org/106353005/diff/350001/Source/modules/performa... Source/modules/performance/WorkerPerformance.h:53: PassRefPtr<MemoryInfo> memory() const; Change this to PassRefPtrWillBeRawPtr to make it Oilpan friendly.
https://codereview.chromium.org/106353005/diff/350001/LayoutTests/http/tests/... File LayoutTests/http/tests/w3c/webperf/resources/worker.js (right): https://codereview.chromium.org/106353005/diff/350001/LayoutTests/http/tests/... LayoutTests/http/tests/w3c/webperf/resources/worker.js:16: postMessage(JSON.stringify(results)); On 2014/03/20 20:19:12, sof wrote: > why do we need to indirect via JSON and not rely on structured cloning? seems structured cloning does not work on performance.memory object. so manually serialize it here. https://codereview.chromium.org/106353005/diff/350001/Source/modules/performa... File Source/modules/performance/WorkerPerformance.h (right): https://codereview.chromium.org/106353005/diff/350001/Source/modules/performa... Source/modules/performance/WorkerPerformance.h:53: PassRefPtr<MemoryInfo> memory() const; On 2014/03/20 20:19:12, sof wrote: > Change this to PassRefPtrWillBeRawPtr to make it Oilpan friendly. Oh, yes, I haven't aware this change, thanks.
@kinuko, patchset 6 shows the RuntimeEnabledFeatures way, I think it is ok, as once browser started, the switch will be be applied to all render and workers process, won't changed for a specified worker, do you have concerns? thanks for your help. thanks Pan
https://codereview.chromium.org/106353005/diff/350001/LayoutTests/http/tests/... File LayoutTests/http/tests/w3c/webperf/resources/worker.js (right): https://codereview.chromium.org/106353005/diff/350001/LayoutTests/http/tests/... LayoutTests/http/tests/w3c/webperf/resources/worker.js:16: postMessage(JSON.stringify(results)); On 2014/03/24 07:31:18, Pan wrote: > On 2014/03/20 20:19:12, sof wrote: > > why do we need to indirect via JSON and not rely on structured cloning? > seems structured cloning does not work on performance.memory object. so manually > serialize it here. It's a bit stronger than that, every result in the webperf/ testsuite is converted to JSON. Can't you selectively serialize performance.memory instead?
https://codereview.chromium.org/106353005/diff/350001/LayoutTests/http/tests/... File LayoutTests/http/tests/w3c/webperf/resources/worker.js (right): https://codereview.chromium.org/106353005/diff/350001/LayoutTests/http/tests/... LayoutTests/http/tests/w3c/webperf/resources/worker.js:16: postMessage(JSON.stringify(results)); > > It's a bit stronger than that, every result in the webperf/ testsuite is > converted to JSON. Can't you selectively serialize performance.memory instead? thanks, you can see we don't distinguish performance.memory and other objects in this code. Other related test suit in webperf/ folder is for performance.now(). is there any concern we use JSON here :) Pan
https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... File LayoutTests/http/tests/misc/performance-memory-in-dedicated-worker.html (right): https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... LayoutTests/http/tests/misc/performance-memory-in-dedicated-worker.html:16: 'self.performance.memory;', Change this to 'JSON.stringify(self.performance.memory);' https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... LayoutTests/http/tests/misc/performance-memory-in-dedicated-worker.html:23: var jsHeap = results[2]; Move this down to just before it is used & explicitly call JSON.parse() on it. https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... File LayoutTests/http/tests/misc/performance-memory-in-shared-worker.html (right): https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... LayoutTests/http/tests/misc/performance-memory-in-shared-worker.html:16: 'self.performance.memory;', Change this to 'JSON.stringify(self.performance.memory);'. https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... LayoutTests/http/tests/misc/performance-memory-in-shared-worker.html:23: var jsHeap = JSON.parse(event.data).pop(); Move this down 3 lines. https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... File LayoutTests/http/tests/w3c/webperf/resources/worker.js (right): https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... LayoutTests/http/tests/w3c/webperf/resources/worker.js:16: postMessage(JSON.stringify(results)); Can be reverted. https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... LayoutTests/http/tests/w3c/webperf/resources/worker.js:24: port.postMessage(JSON.stringify(results)); Can be reverted. https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... File LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-dedicated-basic.html (right): https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-dedicated-basic.html:25: var results = JSON.parse(event.data); No longer needed. https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... File LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-shared-basic.html (right): https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-shared-basic.html:25: var results = JSON.parse(event.data); Revert https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-shared-basic.html:35: var workerTime = JSON.parse(event.data); Revert
On 2014/03/24 07:54:43, sof wrote: > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > File LayoutTests/http/tests/misc/performance-memory-in-dedicated-worker.html > (right): > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > LayoutTests/http/tests/misc/performance-memory-in-dedicated-worker.html:16: > 'self.performance.memory;', > Change this to 'JSON.stringify(self.performance.memory);' > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > LayoutTests/http/tests/misc/performance-memory-in-dedicated-worker.html:23: var > jsHeap = results[2]; > Move this down to just before it is used & explicitly call JSON.parse() on it. > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > File LayoutTests/http/tests/misc/performance-memory-in-shared-worker.html > (right): > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > LayoutTests/http/tests/misc/performance-memory-in-shared-worker.html:16: > 'self.performance.memory;', > Change this to 'JSON.stringify(self.performance.memory);'. > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > LayoutTests/http/tests/misc/performance-memory-in-shared-worker.html:23: var > jsHeap = JSON.parse(event.data).pop(); > Move this down 3 lines. > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > File LayoutTests/http/tests/w3c/webperf/resources/worker.js (right): > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > LayoutTests/http/tests/w3c/webperf/resources/worker.js:16: > postMessage(JSON.stringify(results)); > Can be reverted. > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > LayoutTests/http/tests/w3c/webperf/resources/worker.js:24: > port.postMessage(JSON.stringify(results)); > Can be reverted. > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > File > LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-dedicated-basic.html > (right): > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-dedicated-basic.html:25: > var results = JSON.parse(event.data); > No longer needed. > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > File > LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-shared-basic.html > (right): > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-shared-basic.html:25: > var results = JSON.parse(event.data); > Revert > > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-shared-basic.html:35: > var workerTime = JSON.parse(event.data); > Revert @sof got point, thanks for your help:) Pan
On 2014/03/25 07:00:09, Pan wrote: .. > https://codereview.chromium.org/106353005/diff/370001/LayoutTests/http/tests/... > > > LayoutTests/http/tests/w3c/webperf/submission/Google/HighResolutionTime/worker-shared-basic.html:35: > > var workerTime = JSON.parse(event.data); > > Revert > > @sof > got point, thanks for your help:) > > Pan Thanks, looks good now.
lgtm2, sorry for my very slow response. I'm glad that we could switch to the simpler impl with RuntimeEnabledFeature.
On 2014/03/25 07:51:27, kinuko wrote: > lgtm2, sorry for my very slow response. I'm glad that we could switch to the > simpler impl with RuntimeEnabledFeature. @kinuko san, never mind, your help is appreciated. @abarth, could you please help review this? thanks Pan
Hi @abarth, could you please to help review this change? thanks Pan
Don't you neat to land this in stages where you first add the public API to enable precise memory data, then teach chromium to call the new API, then land this CL, then delete all the old calls from Chromium, then delete the old setting from Blink? https://codereview.chromium.org/106353005/diff/380001/Source/modules/performa... File Source/modules/performance/WorkerPerformance.cpp (right): https://codereview.chromium.org/106353005/diff/380001/Source/modules/performa... Source/modules/performance/WorkerPerformance.cpp:62: return MemoryInfo::create(); Can you add the same FIXME comment here? We shouldn't be creating a new object every time. https://codereview.chromium.org/106353005/diff/380001/Source/platform/Runtime... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/106353005/diff/380001/Source/platform/Runtime... Source/platform/RuntimeEnabledFeatures.in:88: PreciseMemoryInfo status=experimental This status is not correct. You should leave the status field blank here.
Hi @abarth, @tonyg, We have make the changes of web API and introduce switches in Chromium. This one is the 3rd patches to enable performance.memory in workers. please review. thanks Pan
https://codereview.chromium.org/106353005/diff/450001/Source/core/frame/Setti... File Source/core/frame/Settings.cpp (right): https://codereview.chromium.org/106353005/diff/450001/Source/core/frame/Setti... Source/core/frame/Settings.cpp:143: } This is incorrect. The Settings system and the RuntimeEnabledFeatures system are separate and should not be linked. https://codereview.chromium.org/106353005/diff/450001/Source/core/frame/Setti... File Source/core/frame/Settings.h (right): https://codereview.chromium.org/106353005/diff/450001/Source/core/frame/Setti... Source/core/frame/Settings.h:78: Please do not add more functions to Settings. We now use Settings.in, which generates this code. https://codereview.chromium.org/106353005/diff/450001/Source/core/testing/Int... File Source/core/testing/InternalSettings.h (right): https://codereview.chromium.org/106353005/diff/450001/Source/core/testing/Int... Source/core/testing/InternalSettings.h:73: bool m_originalPreciseMemoryInfoEnabled; Please do not add more members here. This code is now autogenerated by Settings.in. https://codereview.chromium.org/106353005/diff/450001/Source/modules/performa... File Source/modules/performance/WorkerPerformance.cpp (right): https://codereview.chromium.org/106353005/diff/450001/Source/modules/performa... Source/modules/performance/WorkerPerformance.cpp:60: // FIXME: We shall not create a new object every time. Rather than propagating this FIXME, would you be willing to fix it instead?
https://codereview.chromium.org/106353005/diff/450001/Source/core/frame/Setti... File Source/core/frame/Settings.cpp (right): https://codereview.chromium.org/106353005/diff/450001/Source/core/frame/Setti... Source/core/frame/Settings.cpp:143: } On 2014/04/23 18:04:37, abarth wrote: > This is incorrect. The Settings system and the RuntimeEnabledFeatures system > are separate and should not be linked. @abarth, @tonyg, Currently, this setting is used as controller in perf tests. If they are not allowed to be linked, Is it possible to remove this setting, then pass the content_switch to perftests? I'm not familiar with perf test harness. thanks Pan
Presumably perf tests can use a command line flag to trigger this runtime-enabled feature.
On 2014/04/24 14:42:54, abarth wrote: > Presumably perf tests can use a command line flag to trigger this > runtime-enabled feature. @tonyg, @abarth To seamlessly resolve this probelm, I'm thinking: 1, In chrome content_switch, use switch "enable-precise-memory-info", it controls both frame and worker memory info. at the same time teach chrome to use this switch for frame and worker. 2, add "enable-precise-memory-info" cmd line flag for layout test and perf test. 3, remove settings code, and land this patch. Any idea? thanks Pan
On 2014/04/28 13:25:00, Pan wrote: > Any idea? Sounds right to me.
Hi @abarth, @tonyg Dependencies were clear, this change is ready I think. PTAL. thanks Pan https://codereview.chromium.org/106353005/diff/450001/Source/modules/performa... File Source/modules/performance/WorkerPerformance.cpp (right): https://codereview.chromium.org/106353005/diff/450001/Source/modules/performa... Source/modules/performance/WorkerPerformance.cpp:60: // FIXME: We shall not create a new object every time. On 2014/04/23 18:04:37, abarth wrote: > Rather than propagating this FIXME, would you be willing to fix it instead? I'd like to fix it in another change.
lgtm
The CQ bit was checked by pan.deng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pan.deng@intel.com/106353005/490001
Message was sent while issue was closed.
Change committed as 173450
Message was sent while issue was closed.
yurys@chromium.org changed reviewers: + yurys@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/106353005/diff/490001/Source/core/timing/Memo... File Source/core/timing/MemoryInfo.cpp (right): https://codereview.chromium.org/106353005/diff/490001/Source/core/timing/Memo... Source/core/timing/MemoryInfo.cpp:140: DEFINE_STATIC_LOCAL(HeapSizeCache, heapSizeCache, ()); HeapSizeCache should become thread-local as it now can be accessed from worker threads as well as from the main one. I opened a bug on this: crbung.com/445007 |
