Closed Bug 1212360 Opened 4 years ago Closed 4 years ago

Stop doing random work in sessionstore for localStorage changes

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Keywords: addon-compat)

Attachments

(1 file)

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.
Jorge, does the plan sound reasonable as regards addons?
Flags: needinfo?(jorge)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Sounds good to me.
Flags: needinfo?(jorge)
Keywords: addon-compat
https://hg.mozilla.org/mozilla-central/rev/03436e11fb12
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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?
Flags: needinfo?(bzbarsky)
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.
(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.
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?
Flags: needinfo?(bzbarsky)
In any case, that should go in a separate bug.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.