Bug 163576 – WTF should make it easier to create threads that die automatically after inactivity

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 291908 [details]
it begins

[Filip Pizlo]

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 on attachment 291964 [details]
seems to work

This is meant to be ready for review.

[WebKit Commit Bot]

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 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]

Created attachment 291967 [details]
the patch

[Filip Pizlo]

Comment on attachment 291967 [details]
the patch

Already reviewed, did not mean to r?

[WebKit Commit Bot]

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]

Landed in https://trac.webkit.org/changeset/207480

[Geoffrey Garen]

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]

(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]

(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.