Bug 182175 - Disable TLS-based TLCs
Summary: Disable TLS-based TLCs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-26 10:35 PST by Filip Pizlo
Modified: 2018-02-15 15:14 PST (History)
10 users (show)

See Also:


Attachments
the patch (7.35 KB, patch)
2018-01-26 10:38 PST, Filip Pizlo
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-01-26 10:35:11 PST
Patch forthcoming.
Comment 1 Radar WebKit Bug Importer 2018-01-26 10:35:44 PST
<rdar://problem/36914694>
Comment 2 Filip Pizlo 2018-01-26 10:38:37 PST
Created attachment 332384 [details]
the patch
Comment 3 EWS Watchlist 2018-01-26 10:41:22 PST
Attachment 332384 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Platform.h:826:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
ERROR: Source/WTF/wtf/Platform.h:827:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
ERROR: Source/WTF/wtf/Platform.h:828:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Mark Lam 2018-01-26 10:41:26 PST
Comment on attachment 332384 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332384&action=review

r=me

> Source/JavaScriptCore/heap/BlockDirectory.cpp:51
> +BlockDirectory::~BlockDirectory()
> +{
> +}
> +

Is this necessary since it's the default implementation?

> Source/JavaScriptCore/heap/BlockDirectory.h:83
> +    ~BlockDirectory();

Is this necessary?
Comment 5 Mark Lam 2018-01-26 10:42:43 PST
Comment on attachment 332384 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332384&action=review

> Source/WTF/wtf/Platform.h:828
> +// This feature is currently disabled because WebCore will context switch VMs without telling JSC.
> +// FIXME: Re-enable this feature.
> +// https://bugs.webkit.org/show_bug.cgi?id=182173

Please convert these to C comments.  As style bot indicated, C++ comments not allowed in this file.
Comment 6 Filip Pizlo 2018-01-26 10:42:59 PST
(In reply to Mark Lam from comment #4)
> Comment on attachment 332384 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332384&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/heap/BlockDirectory.cpp:51
> > +BlockDirectory::~BlockDirectory()
> > +{
> > +}
> > +
> 
> Is this necessary since it's the default implementation?
> 
> > Source/JavaScriptCore/heap/BlockDirectory.h:83
> > +    ~BlockDirectory();
> 
> Is this necessary?

It's often useful to put debugging print statements in the destructors of GC data structures.  So, I've been making sure that new data structures always have out-of-line destructors.  I added an out-of-line destructor while debugging, and I think it's most useful if I just land it.
Comment 7 Filip Pizlo 2018-01-26 10:43:10 PST
(In reply to Mark Lam from comment #5)
> Comment on attachment 332384 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332384&action=review
> 
> > Source/WTF/wtf/Platform.h:828
> > +// This feature is currently disabled because WebCore will context switch VMs without telling JSC.
> > +// FIXME: Re-enable this feature.
> > +// https://bugs.webkit.org/show_bug.cgi?id=182173
> 
> Please convert these to C comments.  As style bot indicated, C++ comments
> not allowed in this file.

Ooops!  Landing follow-up fix.
Comment 9 Filip Pizlo 2018-02-15 15:14:32 PST
This was landed.