1212360 - Stop doing random work in sessionstore for localStorage changes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Kyle found this while profiling http://nolanlawson.github.io/database-comparison/

We fire MozStorageChanged events for changes to local/session/global(?) storage, but the only in-tree consumer cares just about the sessionstorage case.

There is one single addon in addons mxr that cares about this event: "Cookie Controller".  It should not be hard to update that to listen to separate events for the different storage types in practice it ends up trying to detect which storage was changed anyway, so it will save it some work.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Jorge, does the plan sound reasonable as regards addons?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Split up the MozStorageChanged event into separate events for sessionstorage and localstorage

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

Sounds good to me.

Comment 4

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96c8bc76572a

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03436e11fb12

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03436e11fb12

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Boris, I still lots of jank in the demo page in comment 0 for localstorage even with this patch.  It seems the jank only happens when localstorage starts empty, so I'm wondering if its still related to change events being fired.  Any ideas?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Isn't starting-with-empty a case when we don't preload any localStorage until it is first time used.
So we end up doing something sync when accessing localStorage.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #8)
> Isn't starting-with-empty a case when we don't preload any localStorage
> until it is first time used.
> So we end up doing something sync when accessing localStorage.

The pause is after the for loop completes.  The timing shows ~500ms for the for loop, but the animation pauses for seconds.  So I don't think its sync work.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I do still see jank, yes.  A lot less than before this patch.

I profiled this bit.  I see us spending time on the main thread and on the DOMStorageDBThread.  The first question is whether we ever block the former while the latter is doing stuff or whether I should be able to ignore time spent on the DOMStorageDBThread.

Interestingly, the case when I don't clear shows _no_ time spent on DOMStorageDBThread.

Anyway, on the main thread there are two main places where time is spent.  All percentages that follow are percentages of time on the main thread:

35% (about 500ms in my case) under sets on the DOM proxy which eventually lead to DOMSTorage::SetItem.  Lots of various overhead in the JS engine here (e.g. the actual time under SetItem is only 290ms).  Also of interest under here is the 100ms we spend under BroadcastChangeNotification; 45ms constructing StorageEvents, 23ms constructing strings, 18ms dispatching runnables to the main thread.

Another 36% (518ms) under StorageNotifierRunnable::Run.  This is almost all under NotifyObservers, and most of that (30%/430ms) is under nsGlobalWindow::Observe.  Another 3% is nsObserverList::FillObserverArray.  I bet this part gets _much_ worse as you have more windows or tabs, because it's notifying all of them, right?

In any case, that 30% under nsGlobalWindow::Observe breaks down as follows:

  2% (30ms) self time.
 11% (160ms) nsGlobalWindow::DispatchEvent and things under it.  This is having to walk up
             the entire event target chain, including XUL elements... not doing much as it 
             does, but having to do it.
  8% (107ms) CloneStorageEvent.  Mostly self time, StorageEvent::Constructor, string
             copies.
  5% (66ms) BasePrincipal::Equals.  Turns out, NS_SecurityCompareURIs is not the simplest
            thing in the world.  ;)

Then again, we're spending 18ms under nsGlobalWindow::GetPrincipal under here, and _that_ is a pretty simple thing.  So I think the real issue is just that we call into this code a _lot_.

Is there anything we can do to coalesce these notifications or something?

Comment 11

[Boris Zbarsky [:bzbarsky]]

In any case, that should go in a separate bug.