Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1301543002:
[Oilpan] Remove a raw pointer to PlatformEventController class on Oilpan build (Closed)
|
Created:
5 years, 4 months ago by peria Modified:
5 years, 3 months ago CC:
blink-reviews, mlamouri+watch-blink_chromium.org, mvanouwerkerk+watch_chromium.org, Inactive, timvolodine Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionRemove a raw pointer to a PlatformEventController instance in PlatformEventDispatcher on Oilpan build
BUG=509911
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201313
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Fix tests #
Total comments: 12
Patch Set 3 : Fix tests, again #
Total comments: 8
Patch Set 4 : Work for comments #
Total comments: 8
Patch Set 5 : Use copyToVector #Patch Set 6 : Rebase #
Total comments: 2
Messages
Total messages: 27 (11 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:60001) has been deleted
peria@chromium.org changed reviewers: + oilpan-reviews@chromium.org
PTL
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1301543002/diff/80001/Source/core/frame/Platf... File Source/core/frame/PlatformEventDispatcher.cpp (left): https://codereview.chromium.org/1301543002/diff/80001/Source/core/frame/Platf... Source/core/frame/PlatformEventDispatcher.cpp:35: // while the events are still being dispatched. This comment explains why we have to use a Vector instead of a HashSet. In order to use a HashSet (I think this is a good thing), we need to add code to protect the controller while the events are being dispatched. https://codereview.chromium.org/1301543002/diff/80001/Source/core/frame/Platf... File Source/core/frame/PlatformEventDispatcher.h (left): https://codereview.chromium.org/1301543002/diff/80001/Source/core/frame/Platf... Source/core/frame/PlatformEventDispatcher.h:39: bool m_isDispatching; We cannot simply remove the purge logic. See my comment below.
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
https://codereview.chromium.org/1301543002/diff/80001/Source/core/frame/Platf... File Source/core/frame/PlatformEventDispatcher.cpp (left): https://codereview.chromium.org/1301543002/diff/80001/Source/core/frame/Platf... Source/core/frame/PlatformEventDispatcher.cpp:35: // while the events are still being dispatched. On 2015/08/19 11:12:51, haraken wrote: > > This comment explains why we have to use a Vector instead of a HashSet. > > In order to use a HashSet (I think this is a good thing), we need to add code to > protect the controller while the events are being dispatched. Ah, I understand that the main focus of this comment is the last sentence, that means Dispatcher does not stop/restart listening events even if they have no controllers during a dispatching. 're-entrancy' is not of controllers, but of this dispatcher. https://codereview.chromium.org/1301543002/diff/80001/Source/core/frame/Platf... File Source/core/frame/PlatformEventDispatcher.h (left): https://codereview.chromium.org/1301543002/diff/80001/Source/core/frame/Platf... Source/core/frame/PlatformEventDispatcher.h:39: bool m_isDispatching; On 2015/08/19 11:12:51, haraken wrote: > > We cannot simply remove the purge logic. See my comment below. Acknowledged.
timvolodine@chromium.org changed reviewers: + timvolodine@chromium.org
just a couple of drive-by comments/questions ;) https://codereview.chromium.org/1301543002/diff/80001/Source/core/frame/Platf... File Source/core/frame/PlatformEventDispatcher.cpp (left): https://codereview.chromium.org/1301543002/diff/80001/Source/core/frame/Platf... Source/core/frame/PlatformEventDispatcher.cpp:35: // while the events are still being dispatched. On 2015/08/20 09:35:50, peria wrote: > On 2015/08/19 11:12:51, haraken wrote: > > > > This comment explains why we have to use a Vector instead of a HashSet. > > > > In order to use a HashSet (I think this is a good thing), we need to add code > to > > protect the controller while the events are being dispatched. > > Ah, I understand that the main focus of this comment is the last sentence, > that means Dispatcher does not stop/restart listening events even > if they have no controllers during a dispatching. > 're-entrancy' is not of controllers, but of this dispatcher. yes, the main concern here was that we can execute another addEventListener/removeEventListener inside the event callback which would potentially modify m_controllers and have an impact on the dispatch in notifyControllers. https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:42: // m_controllers can be restructured in addController/removeController run on other threads. what other threads? I thought blink was single-threaded? https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:47: snapshotVector.append(controller); why is it necessary to create both snapshotVector and snapshotSet? https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:51: if (m_controllers.contains(controller)) is it possible to remove and then re-add the same controller during dispatch? in which case it would still be called, while not in the previous implementation. Not sure if this is an issue though. https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:51: if (m_controllers.contains(controller)) also, how expensive is 'contains' here? the notifyControllers code can be high frequency as in the case of gamepad and device motion, so it's important to keep this code efficient.
Thank you for your comments, Tim! https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:42: // m_controllers can be restructured in addController/removeController run on other threads. On 2015/08/20 11:16:17, timvolodine wrote: > what other threads? I thought blink was single-threaded? Oops, it comes from my misunderstanding. Dropped this comment. https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:47: snapshotVector.append(controller); On 2015/08/20 11:16:17, timvolodine wrote: > why is it necessary to create both snapshotVector and snapshotSet? snapshotSet was introduced to avoid accessing newly added controller, and snapshotVector was introduced to access all elements in snapshotSet. For the details of latter one, didUpdateData() may delete the controller itself, and it means an element of snapshotSet is removed instead of turning to a nullptr. Removing an element can cause a reconstruction of HeapHashSet, and then we cannot continue the iteration in the set. https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:47: snapshotVector.append(controller); On 2015/08/20 11:16:17, timvolodine wrote: > why is it necessary to create both snapshotVector and snapshotSet? We may remove using snapshotSet. https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:51: if (m_controllers.contains(controller)) On 2015/08/20 11:16:17, timvolodine wrote: > also, how expensive is 'contains' here? the notifyControllers code can be high > frequency as in the case of gamepad and device motion, so it's important to keep > this code efficient. Hmm, unfortunately I don't know the performance regression/improvement of this method. I think we can simply remove this line, if "controller->didUpdateData()" does not delete other controllers. https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:51: if (m_controllers.contains(controller)) On 2015/08/20 11:16:17, timvolodine wrote: > is it possible to remove and then re-add the same controller during dispatch? in > which case it would still be called, while not in the previous implementation. > Not sure if this is an issue though. I think we can, but am not sure.
PS is updated. PTAL. https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:51: if (m_controllers.contains(controller)) On 2015/08/20 11:16:17, timvolodine wrote: > also, how expensive is 'contains' here? the notifyControllers code can be high > frequency as in the case of gamepad and device motion, so it's important to keep > this code efficient. It must be more expensive than checking a nullptr, but I believe it runs fast enough to be used here.
looks much better :) thanks! https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:47: snapshotVector.append(controller); On 2015/08/21 03:33:45, peria wrote: > On 2015/08/20 11:16:17, timvolodine wrote: > > why is it necessary to create both snapshotVector and snapshotSet? > > We may remove using snapshotSet. yeah looks like it's not really needed https://codereview.chromium.org/1301543002/diff/140001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:51: if (m_controllers.contains(controller)) On 2015/08/21 05:34:19, peria wrote: > On 2015/08/20 11:16:17, timvolodine wrote: > > also, how expensive is 'contains' here? the notifyControllers code can be high > > frequency as in the case of gamepad and device motion, so it's important to > keep > > this code efficient. > > It must be more expensive than checking a nullptr, but I believe it runs fast > enough to be used here. we cannot get rid of 'contains' because of re-entrancy of 'didUpdateData' (via JavaScript). looks like it would be O(1) because you are using a HashSet for m_controllers now. https://codereview.chromium.org/1301543002/diff/160001/Source/core/frame/Plat... File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/1301543002/diff/160001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:32: if (!m_controllers.contains(controller)) I think you can drop this check. m_controllers.remove() seems to already have an early return if the controller is not there (from looking at HashSet implementation) https://codereview.chromium.org/1301543002/diff/160001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:49: // PlatformEventController::didUpdateData() can add/remove elements of m_controllers. this is basically re-entrancy on the JavaScript side i.e. event callback can remove another event listener and its controller for example. technically PlatformEventController::didUpdateData() is pure virtual.. https://codereview.chromium.org/1301543002/diff/160001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:51: // Thus we take a snapshot and restore into a Vector to access all elements. "restore" -> "store" ? https://codereview.chromium.org/1301543002/diff/160001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:52: HeapVector<PlatformEventController*> snapshotVector; you could supply an initial capacity to the vector constructor here for efficiency
https://codereview.chromium.org/1301543002/diff/160001/Source/core/frame/Plat... File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/1301543002/diff/160001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:32: if (!m_controllers.contains(controller)) On 2015/08/21 15:03:32, timvolodine wrote: > I think you can drop this check. m_controllers.remove() seems to already have an > early return if the controller is not there (from looking at HashSet > implementation) Done. https://codereview.chromium.org/1301543002/diff/160001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:49: // PlatformEventController::didUpdateData() can add/remove elements of m_controllers. On 2015/08/21 15:03:32, timvolodine wrote: > this is basically re-entrancy on the JavaScript side i.e. event callback can > remove another event listener and its controller for example. technically > PlatformEventController::didUpdateData() is pure virtual.. Thank you for the explanation. I got it. https://codereview.chromium.org/1301543002/diff/160001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:51: // Thus we take a snapshot and restore into a Vector to access all elements. On 2015/08/21 15:03:32, timvolodine wrote: > "restore" -> "store" ? Done. https://codereview.chromium.org/1301543002/diff/160001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:52: HeapVector<PlatformEventController*> snapshotVector; On 2015/08/21 15:03:32, timvolodine wrote: > you could supply an initial capacity to the vector constructor here for > efficiency Done.
PTAL.
LGTM https://codereview.chromium.org/1301543002/diff/180001/Source/core/frame/Plat... File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/1301543002/diff/180001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:22: Add ASSERT(!m_controllers.contains(controller)). https://codereview.chromium.org/1301543002/diff/180001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:31: { Add ASSERT(m_controllers.contains(controller)). https://codereview.chromium.org/1301543002/diff/180001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:48: HeapVector<PlatformEventController*> snapshotVector; WillBeHeapVector<RawPtrWillBeMember<PlatformEventController>> https://codereview.chromium.org/1301543002/diff/180001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:51: snapshotVector.append(controller); Can we use copyToVector?
https://codereview.chromium.org/1301543002/diff/180001/Source/core/frame/Plat... File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/1301543002/diff/180001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:22: On 2015/08/26 00:28:19, haraken wrote: > > Add ASSERT(!m_controllers.contains(controller)). Done. https://codereview.chromium.org/1301543002/diff/180001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:31: { On 2015/08/26 00:28:19, haraken wrote: > > Add ASSERT(m_controllers.contains(controller)). Done. https://codereview.chromium.org/1301543002/diff/180001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:48: HeapVector<PlatformEventController*> snapshotVector; On 2015/08/26 00:28:19, haraken wrote: > > WillBeHeapVector<RawPtrWillBeMember<PlatformEventController>> Done. https://codereview.chromium.org/1301543002/diff/180001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:51: snapshotVector.append(controller); On 2015/08/26 00:28:19, haraken wrote: > > Can we use copyToVector? Done.
Tim: Could you take a look again?
lgtm thanks! https://codereview.chromium.org/1301543002/diff/220001/Source/core/frame/Plat... File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/1301543002/diff/220001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:22: ASSERT(!m_controllers.contains(controller)); nit: looks like it's fine, because we add the controller upon initial promise resolution, but was initially wondering if the assert could fail when we add the same event listener twice (e.g. battery.addEventListener('levelchange',..)).
https://codereview.chromium.org/1301543002/diff/220001/Source/core/frame/Plat... File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/1301543002/diff/220001/Source/core/frame/Plat... Source/core/frame/PlatformEventDispatcher.cpp:22: ASSERT(!m_controllers.contains(controller)); On 2015/08/27 12:31:53, timvolodine wrote: > nit: looks like it's fine, because we add the controller upon initial promise > resolution, but was initially wondering if the assert could fail when we add the > same event listener twice (e.g. battery.addEventListener('levelchange',..)). Yes, this ASSERT assumes that one controller is not added twice. PlatformEventController::startUpdating() seems to avoid such a case, though.
The CQ bit was checked by peria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1301543002/#ps220001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1301543002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1301543002/220001
Message was sent while issue was closed.
Committed patchset #6 (id:220001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201313 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
