Closed Bug 942601 Opened 11 years ago Closed 7 years ago

Online Path of Exile Passive Skill Tree editor generates SessionStore jank by saving lots of page history

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ehoogeveen, Unassigned)

References

(Blocks 1 open bug, )

Details

(Whiteboard: p=13)

Attachments

(4 files)

(Created in response to bug 934934 comment #18)

Having various Path of Exile character builds open significantly impacts SessionStore. It seems that every time you modify one of these builds, it creates a new entry in the page history, and each entry gets saved. I had two builds open in pinned tabs with lots of history from tweaking each build, and this caused me to experience significant jank whenever the SessionStore file was updated. about:sessionstore showed me that these tabs were causing around 26MiB of SessionStore usage, and Windows' task manager showed 400-500 MiB spikes in memory usage every time the SessionStore file was written.

Opening these builds in new tabs (thereby losing the back button history) then clearing the closed tabs entries in about:sessionstore made SessionStore size drop back down to ~400kiB and made the jank go away (it also lowered average total memory usage by about 100MiB, even beyond eliminating the spikes).

See the example build in the associated URL. You can play around with it by deallocating any of the leaf nodes and putting these points into something else - each change will increase the amount of pages showing up in the back button page history by one.

As I see it there are two problems here:
1) Page history ends up containing a large amount of Passive Skill Tree page instances - about:sessionstore showed over 300 children for each pinned tab. Perhaps the amount of data saved this way could be capped, but going back a long way in a tab's history is also a possible use case.
2) SessionStore janks Firefox significantly while reading or writing all this data - a second or two on my fast PC - and memory usage spikes considerably. I don't know how much of this is done asynchronously right now, but I would expect that
a) Nothing in the page history could have changed, so that data shouldn't need to be updated and
b) If it is necessary to handle these entries every time, at least the inactive content should be done asynchronously (I expect that at least some part of saving *active* content will always be synchronous).
This looks like another case of bug 669603.
Incidentally, loading the page at all makes it do some sort of reload that spawns a page history entry. This happens every time I restart the browser as the pinned tabs are restored, so the usage will creep up slowly without any action on my end being required.

Is there anything the page authors could do to prevent Firefox from storing so much useless data (the URL itself contains all the data needed to load a build), or that Firefox could do to infer that storing the data is unnecessary? If this is an evangelism issue I can try to contact the PoE devs, though it seems unfortunate.
The just landed bug 930967 should have improved the situation a bit. Emanuel, if you have some time, is there a chance you could test with the latest Nightly and confirm?
David, this this bug's patch affect junk accumulating issue? Last month in my SessionStore.js I found Google's "Like" button object from a Verge.com article that I opened in a separate tab and *closed* over two months (!) prior that. SessionStore.js accumulates incredible about of garbage -- children objects that do not have parents since forever. Thus I have to kill my session once a while (otherwise SessionStore.js blows up to 60 MB, makes FF crawl and die, killing my set of tabs beyond repair; FF then can load only blank session as the only "*.bak" file gets corrupted, too).
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #0)
> (Created in response to bug 934934 comment #18)
> As I see it there are two problems here:
> 1) Page history ends up containing a large amount of Passive Skill Tree page
> instances - about:sessionstore showed over 300 children for each pinned tab.
> Perhaps the amount of data saved this way could be capped, but going back a
> long way in a tab's history is also a possible use case.

Filed as bug 943339.

> 2) SessionStore janks Firefox significantly while reading or writing all
> this data - a second or two on my fast PC - and memory usage spikes
> considerably. I don't know how much of this is done asynchronously right
> now, but I would expect that

We're moving towards making everything asynchronous. That's a work in progress, but bug 930967 is a large step forwards.

> a) Nothing in the page history could have changed, so that data shouldn't
> need to be updated and
> b) If it is necessary to handle these entries every time, at least the
> inactive content should be done asynchronously (I expect that at least some
> part of saving *active* content will always be synchronous).

We are caching history data already, but we are nor collecting it asynchronously yet. Hopefully, this should arrive soon.
(In reply to User Dderss from comment #4)
> David, this this bug's patch affect junk accumulating issue? Last month in
> my SessionStore.js I found Google's "Like" button object from a Verge.com
> article that I opened in a separate tab and *closed* over two months (!)
> prior that. SessionStore.js accumulates incredible about of garbage --
> children objects that do not have parents since forever. Thus I have to kill
> my session once a while (otherwise SessionStore.js blows up to 60 MB, makes
> FF crawl and die, killing my set of tabs beyond repair; FF then can load
> only blank session as the only "*.bak" file gets corrupted, too).

Filed as bug 943352.
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #3)
> The just landed bug 930967 should have improved the situation a bit.
> Emanuel, if you have some time, is there a chance you could test with the
> latest Nightly and confirm?

Comparing the 2013-11-23 Nightly (before) and the 2013-11-24 Nightly (after) with a clean profile, I see the 2013-11-24 Nightly consistently generating 15% less total bytes, subframes and children. Aside from that constant factor, they behave the same.

The behavior itself is very interesting: across each restart of the browser, the page seems to generate more subframes and children. This continues after the history state reaches its 50/50 cap, and seems nonlinear. I'll try to graph the results from the 2013-11-24 Nightly. I don't know yet whether it eventually reaches a saturation point, or if it will continue to grow exponentially.
By the way, can you attach the about:sessionrestore info about that tab?
I meant about:sessionstore.
Flags: needinfo?(emanuel.hoogeveen)
This shows the size of sessionstore.js, the number of history entries, the number of subframes and the number of children across 110 reloads of the page. As you can see, the growth slows down for a bit after it gets to 50/50 entries, but picks up again after about 90 reloads.
These are some snapshots of sessionstore.js after a certain number of reloads. As you can see, they compress extremely well, which isn't particularly surprising given that it's the same page over and over again.
From the Open tabs section of about:sessionstore after about 110 loads:

window#  tab#       bytes  storage  form data  history state     subframes/children
1           1  14,070,648        0        517          50/50  384/385 (14612/14711)

At this point the nsISessionStore.getBrowserState() call takes 1374ms on my PC according to about:sessionstore. JSON.parse is still pretty snappy at 38ms.
Flags: needinfo?(emanuel.hoogeveen)
This shows the size increase in sessionstore.js after each reload. Until all 50 entries are used you can probably say this is the size of each new entry - after 50, it's probably the size of the new entry minus the size of the oldest entry. As you can see this initially slows the growth down, but eventually the new entries start growing rapidly enough to overtake the removal of the old ones (assuming those aren't being kept somehow).

For context, the initial size of the sessionstore.js for this page is about 10kiB, which already includes a reload.
One of your snapshots, pretty-printed.
Comment on attachment 8340082 [details]
Pretty-printed 16Mb sessionstore.js

So it looks like each entry is accumulating more and more social media-related (Facebook, twitter) elements?
Seems to be (almost?) the same problem as bug 934935: 13000+ iframes, alternating tweeter and facebook.
Which means that we should resume work on bug 936271.
Emanuel, do you have a way to get in touch and report a bug on Path of Exile?
Blocks: 759986
No special contacts, I'm just a regular user. I can try their support forum though.
Hi David,

I got forwarded your email to contact@grindinggear.com. I'm responsible for the PoE skill tree development. I'm very limited in the amount I can test today since I'm quite busy but here is what I have found so far.

I'm finding it hard to generate a large sessionstore.js

Certainly nowhere near the megabyte range easily, much closer to a few hundred KB. Generally I can make it increase by around 10KB per page load. This was on my most recent test with one tab open and reloading Firefox a few times. 

I then went on to completely remove the Facebook and Twitter links and repeated after which the file size didn't increase.

From what I can tell the skill tree itself isn't doing anything crazy with the history API. I'm not seeing any pushstate triggered on page load. I'm also not storing any extra data with these calls, just an empty object.

I can't actually access about:sessionstore which sounds like it will make this a lot easier to debug, how do I get that? I just get "The address isn't valid".

I'll investigate the Facebook/Twitter stuff more when I get a chance, but if there is anything in particular you would like me to look into, please let me know. I'm curious as to what you think we could be doing incorrectly, so I know where to look.
(In reply to Dylan Arnold from comment #20)

Thanks for joining, Dylan.

> I can't actually access about:sessionstore which sounds like it will make
> this a lot easier to debug, how do I get that? I just get "The address isn't
> valid".

It's an extension: https://addons.mozilla.org/en-US/firefox/addon/about-sessionstore/

> I'll investigate the Facebook/Twitter stuff more when I get a chance, but if
> there is anything in particular you would like me to look into, please let
> me know. I'm curious as to what you think we could be doing incorrectly, so
> I know where to look.

Emanuel, can you see with Dylan if there's a simple way to reproduce your session restore situation?
I tested on a fresh profile with a recent build of Firefox Nightly ( https://nightly.mozilla.org/ ) - I don't expect that Nightly will have regressed compared to official, but it's possible.

With Firefox set to "Show my windows and tabs from last time", I then opened the linked build:
http://www.pathofexile.com/passive-skill-tree/AAAAAgABlKDaYmh0-eimVzGe2E0n7Rku1I-53e8OxthYY1BQjM_2SF8_rKpyqXzZQarfvxRxpzAB5zboYSGE73Tteu9gSwUtxFgn1YTZZU1HfqvFfLvFivAfogAUTQSzAnG-vILkkBGKr_IvkFWnhBo4FCBQR0rIWK-18qlueA064fJFTeM1kv6PwBpZ8_jr9zJuqudjiPGsWew4Otg_JypNFSAs6e960359df66g9vuDiP27_DdDfzFZ6CHdkp9_EvOcVb6WkhsFp_LGJH3viXfG_qkGb6KAdzi6j38fLgpLowGpKxuab6n0k3AD0Zp_gqpaA==
and pinned the tab, with about:sessionstore open in another tab, and checked the Highlight similar skills and Highlight shortest paths options (this increases the amount of form data saved a little).

After that, closed the browser and opened it again, let it load, then refreshed about:sessionstore and copied the data under "Open tabs:" to a text file. I repeated this step ~100 times to reach the size reported in the first comment. What I saw was that the sessionstore file started growing faster and faster up until it reached its cap of 50 history entries, grew more slowly after that until around 90 reloads, then started growing faster again.

I haven't tested just modifying a build without restarting the browser to see if this behaves any differently (if the social buttons keep the same session ID, maybe the growth is slower?), but that's what I usually do and how I eventually ended up with a ~13MiB sessionstore entry per tab in comment #0. So note that the problem isn't so much that the file grows rapidly - it's that it never stops and slowly picks up speed, so you eventually end up in a bad situation.
Also note that if you're testing with a new profile with About sessionstore installed, you can replace your sessionstore.js with one of the files from the archive I attached in comment #11 (just rename them). For instance with sessionstore-100.js (after 100 reloads) you should see the file growing with 150-200kiB per reload.
David, in bug 936271 comment #13 billm noted that dynamically generated iframes created after the load event do not get restored. When tabs are restored, to what extent do their history entries get loaded too? Is it possible that the dynamic iframes are created as each history entry is restored, and we then save both the new and old (unrestored) iframes?
I did a bit more testing. The build you sent me triggers the skill tree to open in full screen mode. I found that when a build is loaded in full screen mode it calls window.history.pushState({}, "", url) unnecessarily. 

I have commit a fix to remove this and it should be in a patch within the next week. There is a bit of the file size increase coming from this extra call. However I found that there is more to the size increase than coming from this bug. In order for the file size to stabilize between browser restarts I had to remove all the social buttons as well.

I have a feeling that that part of the equation may be beyond my control but do let me know if there is something else I can look at.
An update here: I no longer see the amount of subframes or children increasing across browser restarts, which should fix the major problem here. I believe this was due to a fix on their end to the fullscreen switch. I haven't retested modifying a build (which is at least a deliberate action) yet.
On our side, we are progressing towards not collecting/storing dynamically generated iframes.
Also, bug 944694 may be related to the issue at hand.
Although I haven't retested recently (I haven't played PoE in a while so no reason to keep a character build pinned), I think this is mostly fixed:
1) The skill tree editor no longer loads twice to go into fullscreen mode (this was a fix or workaround made to the website itself), so the amount of session history entries no longer goes up across restarts of the browser.
2) As of bug 936271 we no longer save dynamically generated iframes, which probably cuts down on the infinitely growing chain of children I was seeing here (I haven't tested to confirm this).
3) As of bug 943339 we only save up to 10 back button session history entries to sessionstore.js, so the maximum number of entries we save should be 5x less.

I'd like to wait for bug 944694 to be fixed before I retest this, but we should already be in much better shape.
Blocks: fxdesktopbacklog
No longer blocks: fxdesktoptriage
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=13
So, Emanuel, do you think we can close this?
Flags: needinfo?(emanuel.hoogeveen)
Oh, almost certainly; I didn't realize this was still open. If my comment from 3 years ago is to be believed, this should be fully fixed with bug 944694.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(emanuel.hoogeveen)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: