Bug 134109

Summary: Increase priority on SharedTimer source
Product: WebKit Reporter: Andre Moreira Magalhaes <andrunko>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: REOPENED ---    
Severity: Normal CC: bugs-noreply, clopez, commit-queue, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch with proper changelog none

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.