Bug 163576

Summary: WTF should make it easier to create threads that die automatically after inactivity
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: Web Template FrameworkAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 163562, 163615    
Attachments:
Description Flags
it begins
none
seems to work
kling: review+
the patch none

Filip Pizlo
Reported 2016-10-17 17:55:25 PDT
Patch forthcoming.
Attachments
it begins (25.00 KB, patch)
2016-10-17 18:38 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
seems to work (30.95 KB, patch)
2016-10-18 11:55 PDT, Filip Pizlo 		kling: review+
DetailsFormatted DiffDiff
the patch (31.62 KB, patch)
2016-10-18 12:23 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2016-10-17 18:38:02 PDT
Created attachment 291908 [details] it begins
Filip Pizlo
Comment 2 2016-10-18 11:55:20 PDT
Created attachment 291964 [details] seems to work Obviously, I have to test this more. But it seems to work so far. It's really exciting to see it stop and then restart its threads.
Filip Pizlo
Comment 3 2016-10-18 12:01:26 PDT
Comment on attachment 291964 [details] seems to work This is meant to be ready for review.
WebKit Commit Bot
Comment 4 2016-10-18 12:03:58 PDT
Attachment 291964 [details] did not pass style-queue: ERROR: Source/WTF/wtf/AutomaticThread.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:203: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/AutomaticThread.cpp:100: 'preserveThisForThread' is incorrectly named. It should be named 'protectedThis'. [readability/naming/protected] [4] ERROR: Source/WTF/wtf/AutomaticThread.cpp:107: 'preserveThisInThread' is incorrectly named. It should be named 'protector' or 'protectedPreserveThisForThread'. [readability/naming/protected] [4] ERROR: Source/WTF/wtf/AutomaticThread.cpp:120: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 5 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Andreas Kling
Comment 5 2016-10-18 12:16:46 PDT
Comment on attachment 291964 [details] seems to work r=me. This is a really nice and clean solution to an issue we've had for ages. Can't wait to see some nice and short crash dumps :)
Filip Pizlo
Comment 6 2016-10-18 12:23:52 PDT
Created attachment 291967 [details] the patch
Filip Pizlo
Comment 7 2016-10-18 12:24:06 PDT
Comment on attachment 291967 [details] the patch Already reviewed, did not mean to r?
WebKit Commit Bot
Comment 8 2016-10-18 12:26:27 PDT
Attachment 291967 [details] did not pass style-queue: ERROR: Source/WTF/wtf/AutomaticThread.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/ParallelHelperPool.cpp:203: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/AutomaticThread.cpp:100: 'preserveThisForThread' is incorrectly named. It should be named 'protectedThis'. [readability/naming/protected] [4] ERROR: Source/WTF/wtf/AutomaticThread.cpp:107: 'preserveThisInThread' is incorrectly named. It should be named 'protector' or 'protectedPreserveThisForThread'. [readability/naming/protected] [4] ERROR: Source/WTF/wtf/AutomaticThread.cpp:120: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 5 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Filip Pizlo
Comment 9 2016-10-18 13:20:10 PDT
Landed in https://trac.webkit.org/changeset/207480
Geoffrey Garen
Comment 10 2016-10-18 14:08:28 PDT
Comment on attachment 291967 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=291967&action=review Aww yiss! > Source/WTF/wtf/AutomaticThread.cpp:130 > + double timeout = monotonicallyIncreasingTime() + 1; This probably needs to be a bit longer. FWIW, experiment shows that libdispatch uses 4s.
Filip Pizlo
Comment 11 2016-10-18 19:38:44 PDT
(In reply to comment #10) > Comment on attachment 291967 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291967&action=review > > Aww yiss! > > > Source/WTF/wtf/AutomaticThread.cpp:130 > > + double timeout = monotonicallyIncreasingTime() + 1; > > This probably needs to be a bit longer. FWIW, experiment shows that > libdispatch uses 4s. If this patch is not a regression, then I think 1sec is good. So far, I'm not seeing any hints of regression. It was neutral when I tested locally, and the bots that have come back with data agree that it is neutral.
Filip Pizlo
Comment 12 2016-10-18 19:39:19 PDT
(In reply to comment #11) > (In reply to comment #10) > > Comment on attachment 291967 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=291967&action=review > > > > Aww yiss! > > > > > Source/WTF/wtf/AutomaticThread.cpp:130 > > > + double timeout = monotonicallyIncreasingTime() + 1; > > > > This probably needs to be a bit longer. FWIW, experiment shows that > > libdispatch uses 4s. > > If this patch is not a regression, then I think 1sec is good. So far, I'm > not seeing any hints of regression. It was neutral when I tested locally, > and the bots that have come back with data agree that it is neutral. Of course, that may change with https://bugs.webkit.org/show_bug.cgi?id=163615.
Note You need to log in before you can comment on or make changes to this bug.