1250502 - One observer registered for every document in order to listen for service-worker-get-client

Status: RESOLVED WONTFIX

(Core :: DOM: Service Workers, defect)

Description

[:Gijs (he/him)]

This is listed in bug 1195386, but it seems to happen irrespective of whether e10s is on, and I'm wondering if there's a relatively easy efficiency win to be had here. Here's my thinking, and you can tell me if I just misunderstand the code or whatever, as I'm not actually familiar with service workers.

AIUI, right now, whenever a document is created it registers itself as an observer of the notification "service-worker-get-client" (specifically, from nsDocument::Init).

When the notification is fired, it looks up its client ID using GetOrCreateID, which as the name suggests, might create the ID, which is then a randomly generated UUID for each document.

The only "notifier"/sender of this notification is ServiceWorkerManager. It uses the notification to get a list of the document(s?) that have a particular client ID.

Now... AIUI that can only work if the document in question has an ID that the caller can predict. That is never the case if the observer notification finds the document without an ID.

Logically, that means the only case where we care about the notification is when an ID is set on the document. Which then makes me think that it would make more sense to add the observer only when the client ID is actually set on the document (either using SetID or GenerateID or an earlier call to GetOrCreateID), so that we don't bother registering all these observers in profiles with a lot of tabs.

Does that make sense, or am I missing something?

Comment 1

[Josh Matthews [:jdm]]

The things you write do make sense to me, but I'm going to get Ehsan's input here.

Comment 2

[(no longer active)]

We can't register the observer lazily only if we have an ID since ServiceWorkerManager::GetAllClients() uses the registered observers to find existing documents that fall under a specific scope.  If we did what you suggest, we would have to maintain a list of documents in the ServiceWorkerManager which I really don't want to do.  Is this observer causing problems?

Comment 3

[:Gijs (he/him)]

(In reply to :Ehsan Akhgari from comment #2)
> We can't register the observer lazily only if we have an ID since
> ServiceWorkerManager::GetAllClients() uses the registered observers to find
> existing documents that fall under a specific scope.  If we did what you
> suggest, we would have to maintain a list of documents in the
> ServiceWorkerManager which I really don't want to do.

Ah, I missed that bit of things. What's the reasoning not to want to have to maintain said list and delegating it to the observer service? It seems to sacrifice the efficiency of the actual lookup-by-id, because now it's always O(N) where N is all documents, which seems avoidable...

>  Is this observer
> causing problems?

No, it just seems like high overhead when there is a large number of documents, see the blocking bug.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Does any of this work across process boundaries?  For example, with multiple content processes.  Just wondering if all this will have to change with the e10s refactor anyways.

Comment 5

[Andrew Overholt [:overholt]]

ni?(jdm) for comment 4.

Comment 6

[(no longer active)]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to :Ehsan Akhgari from comment #2)
> > We can't register the observer lazily only if we have an ID since
> > ServiceWorkerManager::GetAllClients() uses the registered observers to find
> > existing documents that fall under a specific scope.  If we did what you
> > suggest, we would have to maintain a list of documents in the
> > ServiceWorkerManager which I really don't want to do.
> 
> Ah, I missed that bit of things. What's the reasoning not to want to have to
> maintain said list and delegating it to the observer service?

No strong reason, mostly trying to a avoid coming up with more infrastructure if possible.

> It seems to
> sacrifice the efficiency of the actual lookup-by-id, because now it's always
> O(N) where N is all documents, which seems avoidable...

Hmm, which lookup by ID are you mentioning here?  I'm still not 100% sure if I understand the problem.  I would appreciate a more thorough explanation.  :-)

> >  Is this observer
> > causing problems?
> 
> No, it just seems like high overhead when there is a large number of
> documents, see the blocking bug.

Based on the blocking bug, I'm assuming you're mentioning memory overhead.  But I'm not sure how we would avoid it.  nsObserverList uses two words per object (in ObserverRef).  This means 2K words if you have 1K documents, for example, which doesn't sound terrible to me.  If we resort to doing something like tagging the least significant bit of the pointer, we can bring that down to 1 word per document which is the lowest possible memory usage here.  But I suspect that is not the issue you're talking about...

(In reply to Ben Kelly [:bkelly] from comment #4)
> Does any of this work across process boundaries?  For example, with multiple
> content processes.  Just wondering if all this will have to change with the
> e10s refactor anyways.

No, that's unrelated.  This is all broken with multiple content processes, see bug 1229501.

Comment 7

[:Gijs (he/him)]

(In reply to :Ehsan Akhgari from comment #6)
> (In reply to :Gijs Kruitbosch from comment #3)
> > (In reply to :Ehsan Akhgari from comment #2)
> > > We can't register the observer lazily only if we have an ID since
> > > ServiceWorkerManager::GetAllClients() uses the registered observers to find
> > > existing documents that fall under a specific scope.  If we did what you
> > > suggest, we would have to maintain a list of documents in the
> > > ServiceWorkerManager which I really don't want to do.
> > 
> > Ah, I missed that bit of things. What's the reasoning not to want to have to
> > maintain said list and delegating it to the observer service?
> 
> No strong reason, mostly trying to a avoid coming up with more
> infrastructure if possible.
> 
> > It seems to
> > sacrifice the efficiency of the actual lookup-by-id, because now it's always
> > O(N) where N is all documents, which seems avoidable...
> 
> Hmm, which lookup by ID are you mentioning here?  I'm still not 100% sure if
> I understand the problem.  I would appreciate a more thorough explanation. 
> :-)

https://dxr.mozilla.org/mozilla-central/rev/2b5237c178ea02133a777396c24dd2b713f2b8ee/dom/workers/ServiceWorkerManager.cpp#4028-4039

This sends an observer notification, which hits all N documents. But we're looking up a document by its client ID, so only 1 document is going to assign itself to the interface pointer (ifptr). If the service worker manager listened for documents being created and the documents published their own IDs, it could keep a map from document IDs to weakrefs to docs, or something of that sort.

Comment 8

[(no longer active)]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to :Ehsan Akhgari from comment #6)
> > (In reply to :Gijs Kruitbosch from comment #3)
> > > (In reply to :Ehsan Akhgari from comment #2)
> > > > We can't register the observer lazily only if we have an ID since
> > > > ServiceWorkerManager::GetAllClients() uses the registered observers to find
> > > > existing documents that fall under a specific scope.  If we did what you
> > > > suggest, we would have to maintain a list of documents in the
> > > > ServiceWorkerManager which I really don't want to do.
> > > 
> > > Ah, I missed that bit of things. What's the reasoning not to want to have to
> > > maintain said list and delegating it to the observer service?
> > 
> > No strong reason, mostly trying to a avoid coming up with more
> > infrastructure if possible.
> > 
> > > It seems to
> > > sacrifice the efficiency of the actual lookup-by-id, because now it's always
> > > O(N) where N is all documents, which seems avoidable...
> > 
> > Hmm, which lookup by ID are you mentioning here?  I'm still not 100% sure if
> > I understand the problem.  I would appreciate a more thorough explanation. 
> > :-)
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 2b5237c178ea02133a777396c24dd2b713f2b8ee/dom/workers/ServiceWorkerManager.
> cpp#4028-4039
> 
> This sends an observer notification, which hits all N documents. But we're
> looking up a document by its client ID, so only 1 document is going to
> assign itself to the interface pointer (ifptr). If the service worker
> manager listened for documents being created and the documents published
> their own IDs, it could keep a map from document IDs to weakrefs to docs, or
> something of that sort.

Yeah, that's true.  But I don't think this is worth optimizing, since we'd need to pay the same memory cost to have a list of all documents anyway.  This is not hot code at all.

Comment 9

[:Gijs (he/him)]

(In reply to :Ehsan Akhgari from comment #8)
> Yeah, that's true.  But I don't think this is worth optimizing, since we'd
> need to pay the same memory cost to have a list of all documents anyway. 
> This is not hot code at all.

OK, then let's close this out.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Ehsan, are you aware that ServiceWorkerManager already maintains a list of all documents in order to implement Clients.claim()?

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?from=ServiceWorkerManager.cpp#1953

Comment 11

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #10)
> Ehsan, are you aware that ServiceWorkerManager already maintains a list of
> all documents in order to implement Clients.claim()?
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/workers/
> ServiceWorkerManager.cpp?from=ServiceWorkerManager.cpp#1953

No, I didn't know that.  We should probably stop doing that!

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

To be honest, I'm leaning towards going the other way.  My reasons are:

1) It seems very obfuscated to me to hide this list in the observer service.  I think its something that will confuse future maintainers.  Its much easier to understand an explicit list in the SWM.
2) We can better control how we work with the list. For example, the observer pattern will fire the service-worker-get-client at every document in the list even if we've already found a match for the client ID.  With an explicit list we can just terminate the iteration and be done.
3) We can also better clean up cleared weak references if we manager our own list.  The observer service is not the best at this.  (I plan to make SWM hold weak references instead of strong references.)
4) The explicit list pre-dates the use of the observer pattern by about 7 months.

Is there another factor I'm missing which makes an explicit list inappropriate?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Ehsan convinced me to keep the observer list approach over IRC.