|
|
Created:
4 years ago by nhiroki Modified:
3 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, dglazkov+blink, darin-cc_chromium.org, blink-reviews, kinuko+watch, blink-reviews-api_chromium.org, blink-worker-reviews_chromium.org, kenjibaheux Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorker: Enable UseCounter for SharedWorkerGlobalScope
DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo8c/edit#heading=h.g1fo4m6ayjxf
This CL enables UseCounter for SharedWorker. This works as follows:
- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
the worker. For example, when a shared worker connected from 3 documents calls
some API, it's recorded in their UseCounter as if they all called the API:
[Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
(calls API X) ---> [Renderer(Document2)]
---> [Renderer(Document3)]
(each document records use of API X;
the total count of API X is 3)
- When a new document connects to the existing shared worker, its counter is
synchronized with the current use counts of the worker.
[Browser] ---> [Renderer(Document4)]
(new doc, but has the same counts with documents1-3;
the total count of API X is 4)
To achieve the above, this CL...
- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (SharedWorkerHost) so that a document
newly connected to a worker can counts the API use so far.
BUG=376039
Review-Url: https://codereview.chromium.org/2586863002
Cr-Commit-Position: refs/heads/master@{#449196}
Committed: https://chromium.googlesource.com/chromium/src/+/efd1adc4978287cb9f36a91694fc21c482f996e1
Patch Set 1 #Patch Set 2 : WIP #Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : WIP #Patch Set 6 : WIP #Patch Set 7 : remove countDeprecation code path #Patch Set 8 : fix test failures #Patch Set 9 : fix win compile failures #Patch Set 10 : add unit tests #Patch Set 11 : ready to review #
Total comments: 19
Patch Set 12 : address review comments #
Total comments: 6
Patch Set 13 : rebase #Patch Set 14 : window.open() #Patch Set 15 : clean up #Patch Set 16 : add a missing file #Patch Set 17 : replace ExecutionContext with ScriptState #
Total comments: 4
Patch Set 18 : address review comments and fix styles #Patch Set 19 : rename "feature" to "useCounterId" for consistency #Patch Set 20 : tweak #
Total comments: 2
Patch Set 21 : rename use_counter to used_features #Patch Set 22 : rebase #Patch Set 23 : simplify #Dependent Patchsets: Messages
Total messages: 112 (79 generated)
[email protected] changed reviewers: + [email protected]
This seems like a reasonable start, thanks! A few high-level thoughts: 1. There should be no way for a SharedWorker to depend on the behavior of a CSS property (since it has no DOM access), right? We should probably add a CHECK to verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID property) code path is never hit in the worker case (since otherwise we try to keep the CSS and Feature cases of UseCounter consistent). 2. Do you really need separate IPCs for the feature and deprecation? They both boil down to calls to UseCounter::recordMeasurement, differing only in the additional console logging behavior. If you handle the console logging from within the worker's renderer (as I think you're already doing?) then you can just have a single recordMeasurement IPC. 3. In general we try to avoid changing semantics of existing histograms (instead versioning the histogram under a new name) so that an analysis can be performed over time without conflating changes in metrics with changes in how we measure (eg. our usage graphs on chromestatus.com). In http://crbug.com/676837 I'm in the process of transitioning already (see UseCounter::LegacyCounter for the code that preserves the existing semantics). Does anything call WorkerGlobalScope::countFeature today (I couldn'd find it via inspection)? We'll need to add a hook into UseCounter::count somewhere that checks to see if the Frame represents a SharedWorker, right? It looks like perhaps it wouldn't be too hard to follow my existing pattern and have this change impact only the new histogram. I.e. hook into UseCounter::recordMeasurement such that we still call into the m_legacyCounter path, but replace the other path with the call that boils down to an IPC to the browser. Then we can add a new optional "disable legacy" argument to UseCounter::recordMeasurement which we can use to update only the new histogram in the host renderers. When (after a few months of having both) I remove all the "legacy" code/histograms this should simplify nicely.
On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote: > This seems like a reasonable start, thanks! A few high-level thoughts: > > 1. There should be no way for a SharedWorker to depend on the behavior of a CSS > property (since it has no DOM access), right? We should probably add a CHECK to > verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID > property) code path is never hit in the worker case (since otherwise we try to > keep the CSS and Feature cases of UseCounter consistent). > > 2. Do you really need separate IPCs for the feature and deprecation? They both > boil down to calls to UseCounter::recordMeasurement, differing only in the > additional console logging behavior. If you handle the console logging from > within the worker's renderer (as I think you're already doing?) then you can > just have a single recordMeasurement IPC. > > 3. In general we try to avoid changing semantics of existing histograms (instead > versioning the histogram under a new name) so that an analysis can be performed > over time without conflating changes in metrics with changes in how we measure > (eg. our usage graphs on http://chromestatus.com). In http://crbug.com/676837 I'm in > the process of transitioning already (see UseCounter::LegacyCounter for the code > that preserves the existing semantics). > > Does anything call WorkerGlobalScope::countFeature today (I couldn'd find it via > inspection)? Oh sorry, I see it now in UseCounter::count(ExecutionContext*, Feature). And so a change in histogram semantics has already landed in M57 for the DedicatedWorker case. I'll try whipping up a quick prototype of what I'm thinking to keep the change in semantics to the new experimental histograms. > We'll need to add a hook into UseCounter::count somewhere that > checks to see if the Frame represents a SharedWorker, right? It looks like > perhaps it wouldn't be too hard to follow my existing pattern and have this > change impact only the new histogram. I.e. hook into > UseCounter::recordMeasurement such that we still call into the m_legacyCounter > path, but replace the other path with the call that boils down to an IPC to the > browser. Then we can add a new optional "disable legacy" argument to > UseCounter::recordMeasurement which we can use to update only the new histogram > in the host renderers. When (after a few months of having both) I remove all > the "legacy" code/histograms this should simplify nicely.
On 2017/01/02 22:01:45, Rick Byers wrote: > On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote: > > This seems like a reasonable start, thanks! A few high-level thoughts: > > > > 1. There should be no way for a SharedWorker to depend on the behavior of a > CSS > > property (since it has no DOM access), right? We should probably add a CHECK > to > > verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID > > property) code path is never hit in the worker case (since otherwise we try to > > keep the CSS and Feature cases of UseCounter consistent). > > > > 2. Do you really need separate IPCs for the feature and deprecation? They > both > > boil down to calls to UseCounter::recordMeasurement, differing only in the > > additional console logging behavior. If you handle the console logging from > > within the worker's renderer (as I think you're already doing?) then you can > > just have a single recordMeasurement IPC. > > > > 3. In general we try to avoid changing semantics of existing histograms > (instead > > versioning the histogram under a new name) so that an analysis can be > performed > > over time without conflating changes in metrics with changes in how we measure > > (eg. our usage graphs on http://chromestatus.com). In http://crbug.com/676837 > I'm in > > the process of transitioning already (see UseCounter::LegacyCounter for the > code > > that preserves the existing semantics). > > > > Does anything call WorkerGlobalScope::countFeature today (I couldn'd find it > via > > inspection)? > > Oh sorry, I see it now in UseCounter::count(ExecutionContext*, Feature). And so > a change in histogram semantics has already landed in M57 for the > DedicatedWorker case. I'll try whipping up a quick prototype of what I'm > thinking to keep the change in semantics to the new experimental histograms. > > > We'll need to add a hook into UseCounter::count somewhere that > > checks to see if the Frame represents a SharedWorker, right? It looks like > > perhaps it wouldn't be too hard to follow my existing pattern and have this > > change impact only the new histogram. I.e. hook into > > UseCounter::recordMeasurement such that we still call into the m_legacyCounter > > path, but replace the other path with the call that boils down to an IPC to > the > > browser. Then we can add a new optional "disable legacy" argument to > > UseCounter::recordMeasurement which we can use to update only the new > histogram > > in the host renderers. When (after a few months of having both) I remove all > > the "legacy" code/histograms this should simplify nicely. I played with this a bit and it's pretty ugly. I don't think it's worth the cost - see my comment here: https://bugs.chromium.org/p/chromium/issues/detail?id=376039#c32. So you should just keep doing what you were doing before with changing semantics of the existing histograms - sorry for the noise!
The CQ bit was checked by [email protected] 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...
Description was changed from ========== Worker: Enable UseCounter for SharedWorkerGlobalScope BUG=376039 ========== to ========== (WIP) Worker: Enable UseCounter for SharedWorkerGlobalScope This CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser so that a document newly connected to a worker can counts the API use so far. BUG=376039 ==========
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by [email protected] 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 [email protected]
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by [email protected] 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 [email protected]
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by [email protected] 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 [email protected]
Dry run: This issue passed the CQ dry run.
Description was changed from ========== (WIP) Worker: Enable UseCounter for SharedWorkerGlobalScope This CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser so that a document newly connected to a worker can counts the API use so far. BUG=376039 ========== to ========== Worker: Enable UseCounter for SharedWorkerGlobalScope This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, a shared worker connected to 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (record use of API X; the total count is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document1)] ---> [Renderer(Document2)] ---> [Renderer(Document3)] ---> [Renderer(Document4)] (new document, but has the same counts with documents1-3) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - makes IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (WorkerUseCounter) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ==========
Description was changed from ========== Worker: Enable UseCounter for SharedWorkerGlobalScope This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, a shared worker connected to 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (record use of API X; the total count is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document1)] ---> [Renderer(Document2)] ---> [Renderer(Document3)] ---> [Renderer(Document4)] (new document, but has the same counts with documents1-3) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - makes IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (WorkerUseCounter) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ========== to ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, a shared worker connected to 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document1)] ---> [Renderer(Document2)] ---> [Renderer(Document3)] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (WorkerUseCounter) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ==========
The CQ bit was checked by [email protected] 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 [email protected]
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by [email protected] 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...
Thank you for your early comments and sorry for my delayed responses. I think this is now ready to review :) On 2017/01/06 19:50:03, Rick Byers wrote: > On 2017/01/02 22:01:45, Rick Byers wrote: > > On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote: > > > This seems like a reasonable start, thanks! A few high-level thoughts: > > > > > > 1. There should be no way for a SharedWorker to depend on the behavior of a > > CSS > > > property (since it has no DOM access), right? We should probably add a > CHECK > > to > > > verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID > > > property) code path is never hit in the worker case (since otherwise we try > to > > > keep the CSS and Feature cases of UseCounter consistent). I took a quick look at this but didn't understand how to check if it's on WorkerGlobalScope or not. How do we access an execution context in the function? > > > 2. Do you really need separate IPCs for the feature and deprecation? They > > both > > > boil down to calls to UseCounter::recordMeasurement, differing only in the > > > additional console logging behavior. If you handle the console logging from > > > within the worker's renderer (as I think you're already doing?) then you can > > > just have a single recordMeasurement IPC. Good point. Yes, console logging is already done in the worker-side. Removed the message for countDeprecation. > > > 3. In general we try to avoid changing semantics of existing histograms > > (instead > > > versioning the histogram under a new name) so that an analysis can be > > performed > > > over time without conflating changes in metrics with changes in how we > measure > > > (eg. our usage graphs on http://chromestatus.com). In > http://crbug.com/676837 > > I'm in > > > the process of transitioning already (see UseCounter::LegacyCounter for the > > code > > > that preserves the existing semantics). > > > > > > Does anything call WorkerGlobalScope::countFeature today (I couldn'd find it > > via > > > inspection)? > > > > Oh sorry, I see it now in UseCounter::count(ExecutionContext*, Feature). And > so > > a change in histogram semantics has already landed in M57 for the > > DedicatedWorker case. I'll try whipping up a quick prototype of what I'm > > thinking to keep the change in semantics to the new experimental histograms. > > > > > We'll need to add a hook into UseCounter::count somewhere that > > > checks to see if the Frame represents a SharedWorker, right? It looks like > > > perhaps it wouldn't be too hard to follow my existing pattern and have this > > > change impact only the new histogram. I.e. hook into > > > UseCounter::recordMeasurement such that we still call into the > m_legacyCounter > > > path, but replace the other path with the call that boils down to an IPC to > > the > > > browser. Then we can add a new optional "disable legacy" argument to > > > UseCounter::recordMeasurement which we can use to update only the new > > histogram > > > in the host renderers. When (after a few months of having both) I remove > all > > > the "legacy" code/histograms this should simplify nicely. > > I played with this a bit and it's pretty ugly. I don't think it's worth the > cost - see my comment here: > https://bugs.chromium.org/p/chromium/issues/detail?id=376039#c32. So you should > just keep doing what you were doing before with changing semantics of the > existing histograms - sorry for the noise! Acknowledged.
The CQ bit was checked by [email protected] to run a CQ dry run
Patchset #11 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, a shared worker connected to 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document1)] ---> [Renderer(Document2)] ---> [Renderer(Document3)] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (WorkerUseCounter) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ========== to ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, a shared worker connected to 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document1)] ---> [Renderer(Document2)] ---> [Renderer(Document3)] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (WorkerUseCounter) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ==========
[email protected] changed reviewers: + [email protected], [email protected]
+horo, kinuko, can you review this? Thanks.
https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html (right): https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:71: assert_true(isUseCounted(frame3, FetchFeature)); Question for rbyers@: Do iframes in a document share the use counter? |frame3| is created immediately before this line, but its use counter already counted FetchFeature here. Is this expected behavior? I assumed each iframe has its own use counter. To emulate the case where multiple documents connect to the same shared worker, this test creates multiple iframes. If this is expected behavior, we may not be able to test this case by layout tests.
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_... File content/common/worker_use_counter.cc (right): https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_... content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes = kNumberOfFeatures / kBitsPerByte; Do we need to use vector<char> here? Maybe we could just use std::bitset, and bitset::to_string and bitset(string) for serialization? https://codereview.chromium.org/2586863002/diff/220001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.h (right): https://codereview.chromium.org/2586863002/diff/220001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.h:31: // connection is established. The comment needs to be updated. https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:249: static_cast<uint32_t>(feature))); nit: if we do exactly same as countFeature, we can just call it https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebSharedWorkerConnectListener.h (right): https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/web/WebSharedWorkerConnectListener.h:39: class WebSharedWorkerConnectListener { Maybe we should rename this as it's no longer for listening 'connect' (Ok to do it in a separate CL)
Description was changed from ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, a shared worker connected to 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document1)] ---> [Renderer(Document2)] ---> [Renderer(Document3)] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (WorkerUseCounter) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ========== to ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, a shared worker connected to 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (WorkerUseCounter) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ==========
Thank you for the comments. Comment reply only; I'll address them tomorrow. https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_... File content/common/worker_use_counter.cc (right): https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_... content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes = kNumberOfFeatures / kBitsPerByte; On 2017/01/24 13:56:31, kinuko wrote: > Do we need to use vector<char> here? Maybe we could just use std::bitset, and > bitset::to_string and bitset(string) for serialization? I think std::bitset is a good option. Before moving to that, let me confirm 2 things... std::bitset::to_string returns flags as a character string, that is, 1 bit is converted to 1 byte. If UseCoutner::Feature has 2000 features, size of the string is 2KB and every SharedWorker connection / ServiceWorker association causes 2KB data transfer over IPC. Is this acceptable in terms of performance? I feel it'd be acceptable, but I'm not really sure. std::bitset requires a bit length as a type parameter like std::bitset<N>, so it's necessary to get the number of UseCounter::Feature on compile time here. Do you think adding a public header (WebUseCounter.h??) to expose the size of UseCounter::Feature is reasonable?
https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared... File content/browser/shared_worker/shared_worker_service_impl.cc (right): https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:353: ScopedWorkerDependencyChecker checker(this); You don't need this? https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:355: FindSharedWorkerHost(filter->render_process_id(), worker_route_id)) nit: brackets https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:373: FindSharedWorkerHost(filter->render_process_id(), worker_route_id)) nit: brackets https://codereview.chromium.org/2586863002/diff/220001/content/common/view_me... File content/common/view_messages.h (right): https://codereview.chromium.org/2586863002/diff/220001/content/common/view_me... content/common/view_messages.h:542: IPC_MESSAGE_ROUTED1(ViewMsg_WorkerConnected, Please write comment about |features|.
https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_... File content/common/worker_use_counter.cc (right): https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_... content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes = kNumberOfFeatures / kBitsPerByte; On 2017/01/24 15:07:30, nhiroki wrote: > On 2017/01/24 13:56:31, kinuko wrote: > > Do we need to use vector<char> here? Maybe we could just use std::bitset, and > > bitset::to_string and bitset(string) for serialization? > > I think std::bitset is a good option. Before moving to that, let me confirm 2 > things... > > std::bitset::to_string returns flags as a character string, that is, 1 bit is > converted to 1 byte. If UseCoutner::Feature has 2000 features, size of the > string is 2KB and every SharedWorker connection / ServiceWorker association > causes 2KB data transfer over IPC. Is this acceptable in terms of performance? I > feel it'd be acceptable, but I'm not really sure. Usually sending IPC up to 2KB doesn't make a lot difference, but I agree that it might sound a little wasteful. How many features are usually flipped on or off? Could using SmallMap<int, bool> be better? (Then we get IPC serializers for free) > std::bitset requires a bit length as a type parameter like std::bitset<N>, so > it's necessary to get the number of UseCounter::Feature on compile time here. Do > you think adding a public header (WebUseCounter.h??) to expose the size of > UseCounter::Feature is reasonable? I thought about just use kNumberOfFeatures and asserts on the size so that the code can be synchronized.
On 2017/01/25 05:12:52, kinuko wrote: > https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_... > File content/common/worker_use_counter.cc (right): > > https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_... > content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes = > kNumberOfFeatures / kBitsPerByte; > On 2017/01/24 15:07:30, nhiroki wrote: > > On 2017/01/24 13:56:31, kinuko wrote: > > > Do we need to use vector<char> here? Maybe we could just use std::bitset, > and > > > bitset::to_string and bitset(string) for serialization? > > > > I think std::bitset is a good option. Before moving to that, let me confirm 2 > > things... > > > > std::bitset::to_string returns flags as a character string, that is, 1 bit is > > converted to 1 byte. If UseCoutner::Feature has 2000 features, size of the > > string is 2KB and every SharedWorker connection / ServiceWorker association > > causes 2KB data transfer over IPC. Is this acceptable in terms of performance? > I > > feel it'd be acceptable, but I'm not really sure. > > Usually sending IPC up to 2KB doesn't make a lot difference, but I agree that it > might sound a little wasteful. > > How many features are usually flipped on or off? Could using SmallMap<int, > bool> be better? (Then we get IPC serializers for free) Or could this be just std::set<int> for the 'count'ed features? > > std::bitset requires a bit length as a type parameter like std::bitset<N>, so > > it's necessary to get the number of UseCounter::Feature on compile time here. > Do > > you think adding a public header (WebUseCounter.h??) to expose the size of > > UseCounter::Feature is reasonable? > > I thought about just use kNumberOfFeatures and asserts on the size so that the > code can be synchronized.
The CQ bit was checked by [email protected] 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...
Thanks for your comments. Updated. https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared... File content/browser/shared_worker/shared_worker_service_impl.cc (right): https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:353: ScopedWorkerDependencyChecker checker(this); On 2017/01/25 03:45:10, horo wrote: > You don't need this? Done. https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:355: FindSharedWorkerHost(filter->render_process_id(), worker_route_id)) On 2017/01/25 03:45:10, horo wrote: > nit: brackets Done. https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared... content/browser/shared_worker/shared_worker_service_impl.cc:373: FindSharedWorkerHost(filter->render_process_id(), worker_route_id)) On 2017/01/25 03:45:10, horo wrote: > nit: brackets Done. https://codereview.chromium.org/2586863002/diff/220001/content/common/view_me... File content/common/view_messages.h (right): https://codereview.chromium.org/2586863002/diff/220001/content/common/view_me... content/common/view_messages.h:542: IPC_MESSAGE_ROUTED1(ViewMsg_WorkerConnected, On 2017/01/25 03:45:10, horo wrote: > Please write comment about |features|. Done. https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_... File content/common/worker_use_counter.cc (right): https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_... content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes = kNumberOfFeatures / kBitsPerByte; On 2017/01/25 05:12:52, kinuko wrote: > On 2017/01/24 15:07:30, nhiroki wrote: > > On 2017/01/24 13:56:31, kinuko wrote: > > > Do we need to use vector<char> here? Maybe we could just use std::bitset, > and > > > bitset::to_string and bitset(string) for serialization? > > > > I think std::bitset is a good option. Before moving to that, let me confirm 2 > > things... > > > > std::bitset::to_string returns flags as a character string, that is, 1 bit is > > converted to 1 byte. If UseCoutner::Feature has 2000 features, size of the > > string is 2KB and every SharedWorker connection / ServiceWorker association > > causes 2KB data transfer over IPC. Is this acceptable in terms of performance? > I > > feel it'd be acceptable, but I'm not really sure. > > Usually sending IPC up to 2KB doesn't make a lot difference, but I agree that it > might sound a little wasteful. > > How many features are usually flipped on or off? Could using SmallMap<int, > bool> be better? (Then we get IPC serializers for free) I think it's not so big for now because most of features are available only on Document (e.g., DOM, CSS). std::set SGTM. Replaced WorkerUseCounter with std::set. > > std::bitset requires a bit length as a type parameter like std::bitset<N>, so > > it's necessary to get the number of UseCounter::Feature on compile time here. > Do > > you think adding a public header (WebUseCounter.h??) to expose the size of > > UseCounter::Feature is reasonable? > > I thought about just use kNumberOfFeatures and asserts on the size so that the > code can be synchronized. Thanks. std::set does not need the max number, so I didn't add a public header or an assertion. https://codereview.chromium.org/2586863002/diff/220001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.h (right): https://codereview.chromium.org/2586863002/diff/220001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.h:31: // connection is established. On 2017/01/24 13:56:31, kinuko wrote: > The comment needs to be updated. Done. https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:249: static_cast<uint32_t>(feature))); On 2017/01/24 13:56:31, kinuko wrote: > nit: if we do exactly same as countFeature, we can just call it Done. https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebSharedWorkerConnectListener.h (right): https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/web/WebSharedWorkerConnectListener.h:39: class WebSharedWorkerConnectListener { On 2017/01/24 13:56:31, kinuko wrote: > Maybe we should rename this as it's no longer for listening 'connect' > > (Ok to do it in a separate CL) Acknowledged.
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
On 2017/01/24 08:45:47, nhiroki wrote: > Thank you for your early comments and sorry for my delayed responses. I think > this is now ready to review :) > > On 2017/01/06 19:50:03, Rick Byers wrote: > > On 2017/01/02 22:01:45, Rick Byers wrote: > > > On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote: > > > > This seems like a reasonable start, thanks! A few high-level thoughts: > > > > > > > > 1. There should be no way for a SharedWorker to depend on the behavior of > a > > > CSS > > > > property (since it has no DOM access), right? We should probably add a > > CHECK > > > to > > > > verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID > > > > property) code path is never hit in the worker case (since otherwise we > try > > to > > > > keep the CSS and Feature cases of UseCounter consistent). > > I took a quick look at this but didn't understand how to check if it's on > WorkerGlobalScope or not. How do we access an execution context in the function? Yeah it looks not easy - we'd probably need to instrument something more upstream in CSSParserContext. As long as you're confident we shouldn't be parsing CSS in workers then that's good enough for me. > > > > 2. Do you really need separate IPCs for the feature and deprecation? They > > > both > > > > boil down to calls to UseCounter::recordMeasurement, differing only in the > > > > additional console logging behavior. If you handle the console logging > from > > > > within the worker's renderer (as I think you're already doing?) then you > can > > > > just have a single recordMeasurement IPC. > > Good point. Yes, console logging is already done in the worker-side. Removed the > message for countDeprecation. > > > > > 3. In general we try to avoid changing semantics of existing histograms > > > (instead > > > > versioning the histogram under a new name) so that an analysis can be > > > performed > > > > over time without conflating changes in metrics with changes in how we > > measure > > > > (eg. our usage graphs on http://chromestatus.com). In > > http://crbug.com/676837 > > > I'm in > > > > the process of transitioning already (see UseCounter::LegacyCounter for > the > > > code > > > > that preserves the existing semantics). > > > > > > > > Does anything call WorkerGlobalScope::countFeature today (I couldn'd find > it > > > via > > > > inspection)? > > > > > > Oh sorry, I see it now in UseCounter::count(ExecutionContext*, Feature). > And > > so > > > a change in histogram semantics has already landed in M57 for the > > > DedicatedWorker case. I'll try whipping up a quick prototype of what I'm > > > thinking to keep the change in semantics to the new experimental histograms. > > > > > > > We'll need to add a hook into UseCounter::count somewhere that > > > > checks to see if the Frame represents a SharedWorker, right? It looks > like > > > > perhaps it wouldn't be too hard to follow my existing pattern and have > this > > > > change impact only the new histogram. I.e. hook into > > > > UseCounter::recordMeasurement such that we still call into the > > m_legacyCounter > > > > path, but replace the other path with the call that boils down to an IPC > to > > > the > > > > browser. Then we can add a new optional "disable legacy" argument to > > > > UseCounter::recordMeasurement which we can use to update only the new > > > histogram > > > > in the host renderers. When (after a few months of having both) I remove > > all > > > > the "legacy" code/histograms this should simplify nicely. > > > > I played with this a bit and it's pretty ugly. I don't think it's worth the > > cost - see my comment here: > > https://bugs.chromium.org/p/chromium/issues/detail?id=376039#c32. So you > should > > just keep doing what you were doing before with changing semantics of the > > existing histograms - sorry for the noise! > > Acknowledged.
On 2017/01/24 09:54:11, nhiroki wrote: > https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html > (right): > > https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:71: > assert_true(isUseCounted(frame3, FetchFeature)); > Question for rbyers@: Do iframes in a document share the use counter? |frame3| > is created immediately before this line, but its use counter already counted > FetchFeature here. Is this expected behavior? > > I assumed each iframe has its own use counter. To emulate the case where > multiple documents connect to the same shared worker, this test creates multiple > iframes. If this is expected behavior, we may not be able to test this case by > layout tests. Yes iframes share a single UseCounter (note Page::m_useCounter - page refers to all the frames in a tab). Conceptually we want to measure what fraction of "page views" may be impacted by a change (since that's what matters to users) - whether a page has 0 or 100 iframes that shouldn't change it's weight.
This looks good, thank you! Just a couple issues with the test. https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js (right): https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js:5: fetch('notfound').catch(() => port.postMessage('fetched')); please use a feature which is Deprecated (or even one of each). I think you're going to hit a DCHECK for the deprecated case (since UseCounter.cpp today doesn't expect deprecated features to come through the normal path). Perhaps we should just relax that DCHECK if there's not an easy way to separate this case out... https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html (right): https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:12: return frame.contentWindow.internals.isUseCounted( Note that this test is pretty limited - doesn't verify how many counts were increased and what the PageVisits impact was. There are ways of getting the full histogram data out to JavaScript, but honestly for UseCounters I don't think it's worth a huge effort to test more than the basics which you're doing here. Probably better to run a few manual test cases and make sure the histograms look right. For manual testing use either chrome://histograms/Blink.UseCounter.Features (knowing that it can be a little finicky to get it to update - may need to reload the page), or for more precise control run with --enable-stats-collection-bindings and from the devtools console issue commands like JSON.parse(statsCollectionController.getHistogram("Blink.UseCounter.Features")) to poke at the entire histogram for THAT renderer. https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:70: //assert_false(isUseCounted(frame3, FetchFeature)); Instead of using frames (which share a UseCounter) perhaps you can test this with popups (window.open) that you communicate with? IIRC there's some window.internals settings for disabling all the pop-up blocker stuff and ensuring all such new windows get closed when the test finishes.
The CQ bit was checked by [email protected] 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 [email protected]
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by [email protected] 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...
Thanks for your comments. Updated. https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js (right): https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js:5: fetch('notfound').catch(() => port.postMessage('fetched')); On 2017/01/26 03:02:37, Rick Byers wrote: > please use a feature which is Deprecated (or even one of each). I think you're > going to hit a DCHECK for the deprecated case (since UseCounter.cpp today > doesn't expect deprecated features to come through the normal path). Perhaps we > should just relax that DCHECK if there's not an easy way to separate this case > out... You're right! Calling a deprecated feature hits DCHECK. Instead of relaxing the check, the latest patchset ccheck whether a given feature is deprecated and calls countDeprecate in the case. See SharedWorkerRepositoryClientImpl.cpp::countFeature. https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html (right): https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:12: return frame.contentWindow.internals.isUseCounted( On 2017/01/26 03:02:37, Rick Byers wrote: > Note that this test is pretty limited - doesn't verify how many counts were > increased and what the PageVisits impact was. There are ways of getting the > full histogram data out to JavaScript, but honestly for UseCounters I don't > think it's worth a huge effort to test more than the basics which you're doing > here. > > Probably better to run a few manual test cases and make sure the histograms look > right. For manual testing use either > chrome://histograms/Blink.UseCounter.Features (knowing that it can be a little > finicky to get it to update - may need to reload the page), or for more precise > control run with --enable-stats-collection-bindings and from the devtools > console issue commands like > JSON.parse(statsCollectionController.getHistogram("Blink.UseCounter.Features")) > to poke at the entire histogram for THAT renderer. Thank you for the guide! Using the command line flag, I confirmed that (1) PageVisits counts only real page views (not including SharedWorker loads), (2) API use on SharedWorkerGlobalScope is recorded only one time on each connected document, and (3) a counter of a newly connected document is correctly synced with a counter of an existing shared worker. In conclusion, this seems to be working well :) https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:70: //assert_false(isUseCounted(frame3, FetchFeature)); On 2017/01/26 03:02:37, Rick Byers wrote: > Instead of using frames (which share a UseCounter) perhaps you can test this > with popups (window.open) that you communicate with? IIRC there's some > window.internals settings for disabling all the pop-up blocker stuff and > ensuring all such new windows get closed when the test finishes. Good idea. Done. It looks like window.open() works without window.internals settings. add_completion_callback() ensures all windows get closed when the test finishes.
On 2017/01/26 02:28:06, Rick Byers wrote: > On 2017/01/24 08:45:47, nhiroki wrote: > > Thank you for your early comments and sorry for my delayed responses. I think > > this is now ready to review :) > > > > On 2017/01/06 19:50:03, Rick Byers wrote: > > > On 2017/01/02 22:01:45, Rick Byers wrote: > > > > On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote: > > > > > This seems like a reasonable start, thanks! A few high-level thoughts: > > > > > > > > > > 1. There should be no way for a SharedWorker to depend on the behavior > of > > a > > > > CSS > > > > > property (since it has no DOM access), right? We should probably add a > > > CHECK > > > > to > > > > > verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID > > > > > property) code path is never hit in the worker case (since otherwise we > > try > > > to > > > > > keep the CSS and Feature cases of UseCounter consistent). > > > > I took a quick look at this but didn't understand how to check if it's on > > WorkerGlobalScope or not. How do we access an execution context in the > function? > > Yeah it looks not easy - we'd probably need to instrument something more > upstream in CSSParserContext. As long as you're confident we shouldn't be > parsing CSS in workers then that's good enough for me. I'm not 100% sure but I don't think it's called on workers because It's called only from CSSPropertyParser and workers cannot access CSS properties.
On 2017/01/26 02:30:35, Rick Byers wrote: > On 2017/01/24 09:54:11, nhiroki wrote: > > > https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay... > > File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html > > (right): > > > > > https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay... > > third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:71: > > assert_true(isUseCounted(frame3, FetchFeature)); > > Question for rbyers@: Do iframes in a document share the use counter? |frame3| > > is created immediately before this line, but its use counter already counted > > FetchFeature here. Is this expected behavior? > > > > I assumed each iframe has its own use counter. To emulate the case where > > multiple documents connect to the same shared worker, this test creates > multiple > > iframes. If this is expected behavior, we may not be able to test this case by > > layout tests. > > Yes iframes share a single UseCounter (note Page::m_useCounter - page refers to > all the frames in a tab). > > Conceptually we want to measure what fraction of "page views" may be impacted by > a change (since that's what matters to users) - whether a page has 0 or 100 > iframes that shouldn't change it's weight. I see. Thank you for the clarification.
[email protected] changed reviewers: + [email protected]
+dcheng@, can you review following files as an owner? - content/common/view_messages.h - content/common/worker_messages.h - third_party/WebKit/Source/web/* Thanks!
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by [email protected] 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 [email protected]
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by [email protected] 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 [email protected]
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2586863002/diff/340001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.h (right): https://codereview.chromium.org/2586863002/diff/340001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.h:48: void OnWorkerConnected(std::set<uint32_t> features); Nit: const std::set<uint32_t>& https://codereview.chromium.org/2586863002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2586863002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:226: static_assert(UseCounter::NumberOfFeatures < UINT32_MAX, Another option is just to write this in UseCounter.h enum Features : uint32_t { ... };
The CQ bit was checked by [email protected] 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 checked by [email protected] 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 checked by [email protected] 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...
Thanks for your comments. Updated. https://codereview.chromium.org/2586863002/diff/340001/content/renderer/share... File content/renderer/shared_worker/websharedworker_proxy.h (right): https://codereview.chromium.org/2586863002/diff/340001/content/renderer/share... content/renderer/shared_worker/websharedworker_proxy.h:48: void OnWorkerConnected(std::set<uint32_t> features); On 2017/02/03 00:40:40, dcheng wrote: > Nit: const std::set<uint32_t>& Done. https://codereview.chromium.org/2586863002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2586863002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:226: static_assert(UseCounter::NumberOfFeatures < UINT32_MAX, On 2017/02/03 00:40:40, dcheng wrote: > Another option is just to write this in UseCounter.h > > enum Features : uint32_t { > ... > }; Good idea! Added it and removed static_asserts.
LGTM with a naming nit. I know the latest PS renamed feature -> useCounterId, but I think it's clearer to call it feature. For example, the std::set<uint32_t> is currently named |use_counters| but the purpose of this isn't very clear from the variable name. If we called referred to this as feature, we can name the set |used_features|, which makes the meaning of the sets clearer. https://codereview.chromium.org/2586863002/diff/400001/content/browser/shared... File content/browser/shared_worker/shared_worker_host.h (right): https://codereview.chromium.org/2586863002/diff/400001/content/browser/shared... content/browser/shared_worker/shared_worker_host.h:146: std::set<uint32_t> use_counter_; Nit: add a comment on this and note that this is the set of features that the shared worker has used.
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, a shared worker connected to 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (WorkerUseCounter) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ========== to ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, a shared worker connected from 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (SharedWorkerHost) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ==========
https://codereview.chromium.org/2586863002/diff/400001/content/browser/shared... File content/browser/shared_worker/shared_worker_host.h (right): https://codereview.chromium.org/2586863002/diff/400001/content/browser/shared... content/browser/shared_worker/shared_worker_host.h:146: std::set<uint32_t> use_counter_; On 2017/02/03 18:27:50, dcheng wrote: > Nit: add a comment on this and note that this is the set of features that the > shared worker has used. Done.
The CQ bit was checked by [email protected] 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...
Thank you. On 2017/02/03 18:27:50, dcheng wrote: > LGTM with a naming nit. > > I know the latest PS renamed feature -> useCounterId, but I think it's clearer > to call it feature. For example, the std::set<uint32_t> is currently named > |use_counters| but the purpose of this isn't very clear from the variable name. > > If we called referred to this as feature, we can name the set |used_features|, > which makes the meaning of the sets clearer. Yeah, |used_features| sounds much clearer. Renamed them.
rbyers@, ping :)
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, a shared worker connected from 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (SharedWorkerHost) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ========== to ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, when a shared worker connected from 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (SharedWorkerHost) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ==========
Description was changed from ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, when a shared worker connected from 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document connects to the existing shared worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others: [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (SharedWorkerHost) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ========== to ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, when a shared worker connected from 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document connects to the existing shared worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (SharedWorkerHost) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ==========
The CQ bit was checked by [email protected] 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 [email protected]
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_...)
Shoot, sorry I missed this. The test looks awesome now, thank you! LGTM
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected], [email protected] Link to the patchset: https://codereview.chromium.org/2586863002/#ps460001 (title: "simplify")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 460001, "attempt_start_ts": 1486605222529510, "parent_rev": "8499b1584c9c4a0c3b018355f0301a3596d93c2f", "commit_rev": "efd1adc4978287cb9f36a91694fc21c482f996e1"}
Message was sent while issue was closed.
Description was changed from ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, when a shared worker connected from 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document connects to the existing shared worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (SharedWorkerHost) so that a document newly connected to a worker can counts the API use so far. BUG=376039 ========== to ========== Worker: Enable UseCounter for SharedWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for SharedWorker. This works as follows: - API use on SharedWorkerGlobalScope is recorded in all documents connecting to the worker. For example, when a shared worker connected from 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document connects to the existing shared worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) To achieve the above, this CL... - extends the lifetime of SharedWorkerConenctListener and uses it as a proxy between Renderer(Document) and Browser. - introduces new IPC path to notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to Renderer(Document) via Browser. - keeps the current counts in Browser (SharedWorkerHost) so that a document newly connected to a worker can counts the API use so far. BUG=376039 Review-Url: https://codereview.chromium.org/2586863002 Cr-Commit-Position: refs/heads/master@{#449196} Committed: https://chromium.googlesource.com/chromium/src/+/efd1adc4978287cb9f36a91694fc... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:460001) as https://chromium.googlesource.com/chromium/src/+/efd1adc4978287cb9f36a91694fc...
Message was sent while issue was closed.
[email protected] changed reviewers: + [email protected]
Message was sent while issue was closed.
It looks like the tests here assume that MessagePort communication shares the same FIFO as Chrome IPC messages. That happens to be true in the current implementation. I'm trying to make it untrue in a new implementation here: https://codereview.chromium.org/2422793002 There are performance benefits to doing so. I'm trying to figure out the best way to support the use counter tests here that rely on the use counter IPCs happening before MessagePort IPCs. Suggestions welcome :)
Message was sent while issue was closed.
On 2017/02/09 20:33:59, darin (slow to review) wrote: > It looks like the tests here assume that MessagePort communication shares the > same FIFO as Chrome IPC messages. That happens to be true in the current > implementation. I'm trying to make it untrue in a new implementation here: > https://codereview.chromium.org/2422793002 > > There are performance benefits to doing so. I'm trying to figure out the best > way to support the use counter tests here that rely on the use counter IPCs > happening before MessagePort IPCs. Suggestions welcome :) Please see: https://codereview.chromium.org/2658603003/#msg97 I'll make a patch for a mechanism to observe UseCounter changes. |