Make entries in TabChild::sActiveTabs and EventLoopActivation::mEventGroups unique

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Priority: -- → P2
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.
Attachment #8909579 - Flags: review?(wmccloskey)
(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?
Flags: needinfo?(wmccloskey)
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).
Flags: needinfo?(wmccloskey)
(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
Attachment #8909579 - Attachment is obsolete: true
Attachment #8911752 - Flags: review?(wmccloskey)
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
Attachment #8911752 - Flags: review?(wmccloskey) → review+
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
https://hg.mozilla.org/mozilla-central/rev/9e7267c06e4e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.