Bug 134109 - Increase priority on SharedTimer source
Summary: Increase priority on SharedTimer source
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-20 10:34 PDT by Andre Moreira Magalhaes
Modified: 2017-03-11 10:51 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.63 KB, patch)
2014-06-20 10:38 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch with proper changelog (2.73 KB, patch)
2014-06-20 10:42 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Moreira Magalhaes 2014-06-20 10:34:16 PDT
While running webkit on a Debian virtual machine with limited resources I stumbled upon an issue where JS setTimeout callbacks were not being properly invoked due to machine limitations and the usage of a low priority on setTimeout timers.

After discussing the issue with Gustavo (kov), we decided to increase the SharedTimer priority to use the default glib priority. I would like to start a discussion on whether this is the proper way to fix this issue.

Patch to come.
Comment 1 Andre Moreira Magalhaes 2014-06-20 10:38:41 PDT
Created attachment 233433 [details]
Patch
Comment 2 Andre Moreira Magalhaes 2014-06-20 10:42:41 PDT
Created attachment 233434 [details]
Patch with proper changelog

Oops, forgot to update changelog, new patch in.
Comment 3 Gustavo Noronha (kov) 2014-06-27 08:19:13 PDT
Comment on attachment 233434 [details]
Patch with proper changelog

This would be a problem for webkit1, but is a proper fix for the webkit2 model IMO, since we do not compete with GTK+ mainloop sources anymore.
Comment 4 WebKit Commit Bot 2014-06-27 08:51:28 PDT
Comment on attachment 233434 [details]
Patch with proper changelog

Clearing flags on attachment: 233434

Committed r170529: <http://trac.webkit.org/changeset/170529>
Comment 5 WebKit Commit Bot 2014-06-27 08:51:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Carlos Alberto Lopez Perez 2014-07-16 07:23:26 PDT
This caused a performance regression on the test Animation/balls:FrameRate. See: https://bugs.webkit.org/show_bug.cgi?id=134972
Comment 7 Martin Robinson 2014-07-16 08:01:31 PDT
The priority of the timers is very sensitive and can negatively affect animations, so I recommend we roll this out until we understand the isseu.
Comment 8 Andre Moreira Magalhaes 2014-07-16 08:43:16 PDT
(In reply to comment #7)
> The priority of the timers is very sensitive and can negatively affect animations, so I recommend we roll this out until we understand the isseu.

I agree here. I actually opened this bug more to start a discussion on the proper priorities to use for the various timers/sources as we had some issues with JS being starved and thus breaking some webapps that relied on setTimeout calls. This issue only happened in a very specific usecase where we had a browser running in a VM with limited resources.

I vote for reverting this but would like to see your opinion on what is the best approach to solve the issue with JS being starved when running on limited resources.
Comment 9 Carlos Alberto Lopez Perez 2014-07-22 08:07:52 PDT
I reverted r170529 on r171343 <https://trac.webkit.org/r171343> because it caused a ~10% performance regression on the perf test Animation/balls: https://webkit.org/b/134972


Reopening this bug.