File this bug to address the problem found in bug 1395208 comment 3. Otherwise, it's possible to call SchedulerGroup::SetIsRunning() multiple times due to redundant entries in these 2 arrays.

This is to fix the problem found in bug 1395208 comment 3.

Comment on attachment 8909579 [details] [diff] [review] (v1) Patch: Make entries in TabChild::sActiveTabs and EventLoopActivation::mEventGroups unique. Review of attachment 8909579 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. The one thing I'm worried about is that it requires us to allocate a new hashtable for every EventLoopActivation, which is for every event. Could you create a data structure inside nsILabelableRunnable called SchedulerGroupSet? It would have one inline field to store a single SchedulerGroup* in case there's only one. If there are more than one, it would use a hashtable. That way we would allocate much less frequently.

(In reply to Bill McCloskey (:billm) from comment #2) > This looks good. The one thing I'm worried about is that it requires us to > allocate a new hashtable for every EventLoopActivation, which is for every > event. Could you create a data structure inside nsILabelableRunnable called > SchedulerGroupSet? It would have one inline field to store a single > SchedulerGroup* in case there's only one. If there are more than one, it > would use a hashtable. That way we would allocate much less frequently. m... In this case, it will take additional O(n) for every Vsync-notify-message to check "If there are more than one" from TabChild::sActiveTabs because it's possible to have multiple entries in TabChild::sActiveTabs which belong to the same TabGroup. (Assuming that the redundant check is done by another hashtable.) May I know what's our precise concern in allocating a hashtable compared to a nsTArray currently allocated for each EventLoopActivation? (Both nsTArray::mHdr and PLDHashTable::mEntryStore are lazily initialized by default, IIUC) Is it because of the larger size of each EventLoopActivation or because of easier memory fragmentation in the heap? BTW, there is one potential problem I saw in BackgroundChildImpl::GetMessageSchedulerGroups(): http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/ipc/glue/BackgroundChildImpl.cpp#641 It always returns true even there is no affected group available in dom::TabChild::HasActiveTabs(). Is it expected?

After thinking twice, I guess you suggested to have some strcture in nsILabelableRunnable like: class ScheduleGroupSet { RefPtr<SchedulerGroup> single; UniquePtr<HashTable<RefPtr<SchedulerGroup>> multi; } For SchedulerGroup::Runnable, always use |single|. For VSync, always allocate and use |multi| if TabChild::HasActiveTabs(). Is this correct?

> May I know what's our precise concern in allocating a hashtable compared to > a nsTArray currently allocated for each EventLoopActivation? > (Both nsTArray::mHdr and PLDHashTable::mEntryStore are lazily initialized by default, IIUC) > Is it because of the larger size of each EventLoopActivation or because of easier memory fragmentation in the heap? The existing code has the same problem. It's my fault. I just want to make sure we fix it. > BTW, there is one potential problem I saw in BackgroundChildImpl::GetMessageSchedulerGroups(): > http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/ipc/glue/BackgroundChildImpl.cpp#641 > It always returns true even there is no affected group available in dom::TabChild::HasActiveTabs(). > Is it expected? I think that's correct. If there are no active tabs, then we'll behave as if we're dispatching to the SystemGroup. > For SchedulerGroup::Runnable, always use |single|. > For VSync, always allocate and use |multi| if TabChild::HasActiveTabs(). > > Is this correct? I was thinking of something like this: class SchedulerGroupSet { RefPtr<SchedulerGroup> mSingle; Maybe<HashTable<RefPtr<SchedulerGroup>>> mMulti; void Put(SchedulerGroup* aGroup) { if (mSingle) { mMulti.emplace(); mMulti.ref().Put(mSingle); mSingle = nullptr; } if (mMulti.isSome()) { mMulti.ref().Put(aGroup); } else { mSingle = aGroup; } } }; That way, we can use mSingle if there is only one active tab (which I expect will be common).

(In reply to Bill McCloskey (:billm) from comment #5) > I was thinking of something like this: > > class SchedulerGroupSet > { > RefPtr<SchedulerGroup> mSingle; > Maybe<HashTable<RefPtr<SchedulerGroup>>> mMulti; I didn't choose Maybe<T> because I found that Maybe<T>::mStorage[sizeof(T)] is preserved which doesn't resolve the memory usage problem in every EventLoopActivation: http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/mfbt/Maybe.h#88 > That way, we can use mSingle if there is only one active tab (which I expect > will be common). Thanks for advice! Hiding the operation on mSingle/mMuti makes more sense here. In addition, for hiding the iteration on these SchedulerGroups, I'll define 2 additional methods: 1. SchedulerGroupSet::SetIsRunning(bool aIsRunning) for the use in SchedulerImpl::StartEvent/FinishEvent. 2. nsILabelableRunnable::IsReadyToRun() for the use in IsReadyToRun() defined in LabeledEventQueue.cpp.

Update changes in v2: 1. Define nsILabelableRunable::SchedulerGroupSet to lazily initiate the hashtable for collecting multiple instances only when needed. (Note: Maybe<T> is not adopted because Maybe<T>::mStorage always perserves unsigned char[size(T)] even emplace() is not called.) 2. For hiding the iteration on these SchedulerGroups, 2 additional methods are defined: - SchedulerGroupSet::SetIsRunning(bool aIsRunning) for the use in SchedulerImpl::StartEvent()/FinishEvent(). - nsILabelableRunnable::IsReadyToRun() for the use in IsReadyToRun() defined in LabeledEventQueue.cpp. Treeherder result looks good, btw: https://treeherder.mozilla.org/#/jobs?repo=try&revision=175760cec97e98149200631575e210cecdde1783

Comment on attachment 8911752 [details] [diff] [review] (v2) Patch: Make entries in TabChild::sActiveTabs and EventLoopActivation::mEventGroups unique. Review of attachment 8911752 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. I like the API you've chosen for the set data structure. But please see my comment below about Maybe<>. ::: xpcom/threads/nsILabelableRunnable.cpp @@ +12,5 @@ > + > +bool > +nsILabelableRunnable::IsReadyToRun() > +{ > + SchedulerGroupSet groups; Please assert Scheduler::AnyEventRunning() and !Scheduler::UnlabeledEventRunning() here. Otherwise this function won't return valid results. ::: xpcom/threads/nsILabelableRunnable.h @@ +47,5 @@ > + using MultipleSchedulerGroups = > + nsTHashtable<nsRefPtrHashKey<mozilla::SchedulerGroup>>; > + > + RefPtr<mozilla::SchedulerGroup> mSingle; > + mozilla::UniquePtr<MultipleSchedulerGroups> mMulti; I don't think using a pointer makes sense here. It does save a little space (32 bytes of stack space, according to [1]), but it forces us to do an extra allocation when we need mMulti. I think it would be better to use Maybe<> here. The only purpose of Maybe<> is to avoid needing to initialize the bytes when we don't need them. The extra space is not significant, and allocations can be expensive. [1] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/xpcom/ds/PLDHashTable.h#241-243

Address suggestion in comment 8.

Pushed by btseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e7267c06e4e Make entries in TabChild::sActiveTabs and EventLoopActivation::mEventGroups unique. r=billm