eric-bixby / auto-sort-bookmarks-webext

Firefox add-on to sort bookmarks by multiple criteria
GNU General Public License v3.0
112 stars 21 forks source link

Indefinite CPU saturation after about a day's usage #19

Closed blackwind closed 7 years ago

blackwind commented 9 years ago

Auto-Sort Bookmarks now constantly eats CPU, gradually increasing the amount used until it reaches 100% after about a day's usage without restarting Firefox. This occurs in Firefox 42 (and probably earlier) with v2.8 and above, but not v2.7. I came to the conclusion that ASB is at fault because as soon as I disable it, CPU usage returns to normal. I imagine it's only a matter of time now before Mozilla breaks v2.7, so, hoping for a swift resolution here.

Gitoffthelawn commented 9 years ago

What are your settings in Auto-Sort Bookmarks? Do you use automatic or manual sorting?

blackwind commented 9 years ago

about:config

antoyo commented 9 years ago

Since you manually sort bookmarks, it is weird that the CPU usage is increasing: the add-on is not suppose to do anything. Did you exclude folders? Did you see a RAM increase as well? Did you see any error related to auto-sort bookmarks in the browser console (Ctrl-Shift-J)? Did you manually sort the bookmarks and are you sure the sort ended (perhaps the sort is now slower, because it is more complex with the new options). Did you test with the stable version of Firefox (in e10s, the add-on runs in a different process and it is recommended to test the add-on in Firefox stable before testing in Nightly)? Moreover, it seems this is not the only case of 100% cpu usage with e10s (https://bugzilla.mozilla.org/show_bug.cgi?id=1130443, https://bugzilla.mozilla.org/buglist.cgi?quicksearch=e10s+cpu).

antoyo commented 9 years ago

@Gitoffthelawn Did you notice a similar issue on your side?

blackwind commented 9 years ago

Interesting, auto-sort is supposed to be enabled. Maybe v2.7 reverted it for some reason, and that's why v2.7 isn't exhibiting the issue. I've re-enabled it, and I'll keep my eye on things today.

I don't exclude folders.

Did I see a RAM increase? Always, but can it be attributed to ASB? Who knows. All I can say definitively is that, unlike CPU, RAM doesn't drop when I disable the add-on.

No errors in the console.

I haven't tested with stable. I'm 95% sure this happened to me during my time on 41 beta, but didn't want to provide potentially misleading information.

I'm not running e10s in Firefox 42 (which is currently aurora, not nightly).

blackwind commented 9 years ago

v2.7 still looking good with auto-sort enabled.

Gitoffthelawn commented 9 years ago

@antoyo I have not noticed a similar issue.

antoyo commented 9 years ago

I was able to reproduce this issue: In version 2.7, Firefox use 2 to 4 times less CPU than with the latest version of the add-on. I'll profile the code to see where the issue lies. Thanks for reporting this issue.

Gitoffthelawn commented 9 years ago

@antoyo Which code profiler are you going to use?

antoyo commented 9 years ago

The profiler included in Firefox. I found there's an option to show the "Gecko platform data" and it shows the function calls from the add-ons too. After a first try, I didn't find anything suspicious. I'll compare the profile with the version 2.7 to get a better idea.

Gitoffthelawn commented 9 years ago

@antoyo Thanks... I was wondering if others liked it too!

jimmydigital commented 8 years ago

I'm seeing the same problem. Enabling the plugin causes firefox to start using 20% of cpu. Seeing the same behavior with auto or manual sorting. It will also steadily climb requiring a restart of firefox after a day or two. Firefox 38.2.1 Plugin 2.10 and os 10.9.5

eric-bixby commented 8 years ago

I'm seeing extremely high CPU usage with the 2.10.4 release on FF 41.0.2 on a Mac. This happens on startup of FF and makes it unusable. I have to start FF in safe-mode and disable the plugin; switching to manual sort does not help. Update: I'm now on FF 42.0 and I'm no longer getting the high CPU usage (with no descriptions; same problem with descriptions).

antoyo commented 8 years ago

@lwb78750 @bonneyarmstrong @TheRealMarnes Does anyone wants to work on this issue? Thanks.

leaumar commented 8 years ago

@antoyo sorry but I haven't had a chance to sit down and get acquainted with all the code yet :/ There's a lot going on for me atm

leaumar commented 8 years ago

About auto-sort magically getting disabled: I already changed that a while ago. It now just stays on or off depending on how you set it, instead of turning itself off every time the addon gets disabled.

bonneyarmstrong commented 8 years ago

Yes, I am getting my dedicated machine back in the next day or so, and then I'll be setting up the dev environment. At that point I'll have more questions about making sure my dev environment mirrors yours as closely as possible, then doing the first build to make sure everything works from the baseline before I tackle changes. Sorry about the delay.

antoyo commented 8 years ago

@bonneyarmstrong Were you able to setup your development environment? Do you need some help?

By the way, perhaps the Firefox's new memory tool will show something interesting to fix this issue.

eric-bixby commented 8 years ago

FYI... I have over 1200 bookmarks, I noticed (by accident) that the problem went away (for me) when there are no descriptions. Therefore, it could be related to the total memory footprint, or possibly certain characters in the descriptions.

zolikonta commented 8 years ago

My favorite plugin stopped working since the update of FF. That's how I find this plugin.

I wish to auto sort my bookmarks based on visit count, only on the bookmarks toolbar. That means less then 50 bookmarks, still the plugin eats one core of my CPU completely after a while. Disabling the plugin releases CPU usage.

Plugin version: 2.10.4 FF version: 43.0

blackwind commented 8 years ago

v2.7 is officially broken in Firefox 44. Can someone take a look at this sooner rather than later?

blackwind commented 8 years ago

Firefox 44 goes stable in two days. Anyone?

Gitoffthelawn commented 8 years ago

Better be sure to sort your bookmarks now!

@blackwind Did you try it on a fresh profile?

eric-bixby commented 8 years ago

I've been studying the code and running tests. Here's a summary of what I've found: 1) Generators are slow. The original code took 95ms to sort the default bookmarks. My revised version without generators took 45ms. Yes, I understand this was probably done so as to not block the browser; however, I don't think it's needed. Anyway, I'm going to do some cleanup and check-in my updates. 2) Next, I'm going to apply a quick-sort; a little more logic but worth it. 3) Finally, the event-model (when auto-sort enabled) needs some work. This will take a little more time, but seems fixable.

michaelalandover commented 8 years ago

I wonder if size of the bookmarks affects memory use. I have very large set of bookmarks. But I’m very happy with how it is working; true, I have lots of memory.

mike

Michael A. Dover,M.S.S.W., Ph.D. College Associate Lecturer Editor, Reflections: Narratives of Professional Helpinghttp://www.rnoph.org/ CSU Sponsor Representative, Cuyahoga County Conference on Social Welfarehttp://www.naswoh.org/?page=cuyahogaconference School of Social Work, Cleveland State University 1425 Rhodes Tower 1860 E. 22nd St. Cleveland OH 44115-2214 Office: (216)687-3564 Faculty profilehttp://facultyprofile.csuohio.edu/csufacultyprofile/detail.cfm?FacultyID=M_A_DOVER //LinkedIn profilehttps://www.linkedin.com/in/michael-dover-1956914 //Human Needs: Overviewhttp://socialwork.oxfordre.com/view/10.1093/acrefore/9780199975839.001.0001/acrefore-9780199975839-e-554 License: LISW (Ohio), LMSW (Clinical and Macro, Michigan)

eric-bixby commented 8 years ago

Short answer is yes.

Yes bookmarks use memory and more bookmarks means more memory usage. During sorting, additional memory is used, but it should not grow (that much). If there is significant memory growth, then that is a sign of a memory leak bug.

In the case of Auto-Sort-Bookmarks (ASB), when auto-sort is enabled, there is excessive looping. When a change event is detected, this triggers sorting. However, sorting is triggering more change events. This chain reaction results is high CPU/memory usage.

Adding a delay is intended to lessen the chance or impact of this looping. However, the problem still exists non the less.

I do have a solution in the works and plan to work on it this weekend.

michaelalandover commented 8 years ago

So cool, thanks.

mike

Michael A. Dover,M.S.S.W., Ph.D. College Associate Lecturer Editor, Reflections: Narratives of Professional Helpinghttp://www.rnoph.org/ CSU Sponsor Representative, Cuyahoga County Conference on Social Welfarehttp://www.naswoh.org/?page=cuyahogaconference School of Social Work, Cleveland State University 1425 Rhodes Tower 1860 E. 22nd St. Cleveland OH 44115-2214 Office: (216)687-3564 Faculty profilehttp://facultyprofile.csuohio.edu/csufacultyprofile/detail.cfm?FacultyID=M_A_DOVER //LinkedIn profilehttps://www.linkedin.com/in/michael-dover-1956914 //Human Needs: Overviewhttp://socialwork.oxfordre.com/view/10.1093/acrefore/9780199975839.001.0001/acrefore-9780199975839-e-554 License: LISW (Ohio), LMSW (Clinical and Macro, Michigan)

eric-bixby commented 8 years ago

I was able to fix the performance issue. Just need to update the unit tests and do some stress testing. If that passes, then I'll submit the new release to Mozilla for review.

antoyo commented 8 years ago

@eric-bixby Thanks for your work. By the way, if you removed the generators and if the code now freezes the browser when sorting (I mean, no more parallel sorting), you can now use the function runInBatchMode() that will make the save of bookmarks way faster (for many thousand bookmarks, this makes a huge difference).

digitalcircuit commented 8 years ago

@antoyo Your remark seems to be the case... After updating to 2.10.6 with generators removed, it freezes on clicking the Sort button, resulting in a "script unresponsive" warning. I have 12447 items according to the Import and Backup menu.

If needed, I can file a new bug report.

eric-bixby commented 8 years ago

I'll try @antoyo's suggestion and I have some other ideas.

Also, I noticed the auto sort thread is running at a fix rate of 3 seconds. I'll add a fix to make that adjustable. I'm guessing over 12K items takes more than 3 seconds to sort, so you'll probably want to adjust that rate.

digitalcircuit commented 8 years ago

I'm not using automatic sort, hence offering to file a new bug report if needed. Firefox hangs for over 20 seconds when manually sorting, and since the menu is open, Gtk or Firefox locks the mouse/keyboard focus. I haven't timed sorts before now, but they seemed to take around 10-20 seconds on a moderately fast desktop (8×3.5 Ghz, 16 gb RAM). Seems like I'm not the only one with trouble, too.

It's almost certainly caused by 2.10.6 due to working fine before and stopping working afterwards without any Firefox or other addon updates. The runInBatchMode() fix from @antoyo might be enough. Time permitting, I'm willing to test :)

eric-bixby commented 8 years ago

I've done some research and experimentation and determined a possible solution: 1) Remove console.log() statements -- great for debugging but causes delays. 2) Worker (aka ChromeWorker and PromiseWorker) -- creates non UI blocking thread

The worker approach will likely work but has limitations. For example, workers do not have access to the Components object. Therefore, my plan is to split the work up. Main thread will generate list of bookmarks and pass that to the worker thread. Worker thread will perform sorting, then pass back (as promise) to main thread. Finally, main thread applies update.

antoyo commented 8 years ago

Beware of the workers: there are issues with them. For instance, the sort result will be different for different locales because of this bug. From the tests I did a while ago, the slowest part of the code was I/O (writing the results to the places.sql database). That is why I highly suggest you to use runInBatchMode(). If you need help, feel free to ask. Here is a script that I wrote to generate a big bookmarks.html file: that may help you to test the performance of the add-on.

By the way, the places API from the add-on SDK is asynchronous. However, when I tested it a while ago, the sorting was not done in another thread, so it freezed the browser, but it may be a good idea to test it again to see if it is different now that e10s is in Firefox. Nevertheless, this API was really slow to sort bookmarks because it used moveItem instead of setIndex under the hood. I don't know if it is still the case.

There is also the new WebExtension thing that you could test to see if it is asynchronous, but rewriting the add-on with WebExtension would require a big amount of work. By the way, is Firefox able to run extensions from Chrome now since it supports WebExtension? There are some bookmark sorting extension on Chrome.

eric-bixby commented 8 years ago

I'm using a PromiseWorker and I'm not able to reproduce the bug you reported for localCompare(), so either they fixed it or it doesn't apply to PromiseWorker or ChromeWorker.

I agree, the WebExtension thing would be worth exploring also.

Gitoffthelawn commented 8 years ago

@antoyo

By the way, is Firefox able to run extensions from Chrome now since it supports WebExtension?

Some, but not all.

See https://addons.mozilla.org/firefox/addon/chrome-store-foxified/

antoyo commented 8 years ago

I've just made some tests:

I tried the SuperSorted add-on from Chrome (in Firefox thanks to the add-on mentioned by @Gitoffthelawn) and it sorted thousands of bookmarks in around 10 minutes (but the sort was asynchronous, not freezing Firefox). I tried with the current version of ASB and it sorted them in 4 minutes (the sort was synchronous, freezing Firefox). I tried with runInBatchMode (see attached patch) and it sorted them in 30 seconds (still synchronous, freezing Firefox). So it is definitely worth using runInBatchMode.

By the way, I think we can get the best of both world (batch + asynchronous) as this test seems to indicate that the Places asynchronous API has been added in Firefox and it seems to be possible to use it with runInBatchMode.

eric-bixby commented 8 years ago

@antoyo, thanks. I've merged in your fix and see an improvement.

eric-bixby commented 8 years ago

I was finally able to perform a sort within a ChromeWorker. Just need to do some cleanup.

eric-bixby commented 7 years ago

@digitalcircuit, I have version 2.10.7 available. You want to give it try and give some feedback?

michaelalandover commented 7 years ago

Yes. The blocking continues to stifle ability to create new folders.

Mike

Sent from my iPad

On Sep 9, 2016, at 11:03 PM, Eric Bixby notifications@github.com<mailto:notifications@github.com> wrote:

@digitalcircuithttps://github.com/digitalcircuit, I have version 2.10.7 available. You want to give it try and give some feedback?

You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/antoyo/auto-sort-bookmarks/issues/19#issuecomment-246084535, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AT_G1MkE6FZyHVb5qqX8z47htRXSThrdks5qoh4UgaJpZM4F1R90.

digitalcircuit commented 7 years ago

@eric-bixby Testing via the "Load Temporary Addon" option in about:debugging#addons, the new version does successfully complete sorting. All of Firefox freezes for 5-15 second-long intervals, making any interaction almost impossible until the sorting finishes (takes about 1 minute), but it does complete and does not trigger the "unresponsive script" warning.

Certainly improves the situation, but still more impactful on the UI than 2.10.5.

digitalcircuit commented 7 years ago

After trying again, the hangs only appear to happen when there's a lot of bookmarks to sort. When there's nothing to sort, or only 1 or 2 bookmarks, it's fairly quick. Firefox hangs in a similar way (edit: without affecting the entire UI, just bookmark management) when adding/removing bookmarks, so I'm not sure this is the fault of the addon.

2.10.7 seems to be a definite improvement over 2.10.6 in either case.

Pardon the minor comment-spam.

eric-bixby commented 7 years ago

@digitalcircuit Great, thanks for the feedback. I will continue to make improvements.

eric-bixby commented 7 years ago

I believe the original problem described in #19 has been addressed, so I will close it.

I've opened #36 to track the improvement of preventing freezing/blocking.