Bug 200611 - [WTF][JSC] Make JSC and WTF aggressively-fast-malloced
Summary: [WTF][JSC] Make JSC and WTF aggressively-fast-malloced
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 200620
  Show dependency treegraph
 
Reported: 2019-08-10 02:18 PDT by Yusuke Suzuki
Modified: 2019-08-13 11:54 PDT (History)
7 users (show)

See Also:


Attachments
Patch (161.07 KB, patch)
2019-08-10 02:33 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.52 KB, patch)
2019-08-10 02:44 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.46 KB, patch)
2019-08-10 02:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (165.16 KB, patch)
2019-08-10 03:04 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (165.82 KB, patch)
2019-08-10 19:44 PDT, Yusuke Suzuki
sbarati: review+
ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-08-10 02:18:37 PDT
[WTF][JSC] Make JSC and WTF aggressively-fast-malloced
Comment 1 Yusuke Suzuki 2019-08-10 02:33:12 PDT
Created attachment 376005 [details]
Patch
Comment 2 Build Bot 2019-08-10 02:36:38 PDT
Attachment 376005 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Optional.h:585:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 3 in 188 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Yusuke Suzuki 2019-08-10 02:44:59 PDT
Created attachment 376006 [details]
Patch
Comment 4 Build Bot 2019-08-10 02:46:51 PDT
Attachment 376006 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Optional.h:585:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 3 in 195 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Yusuke Suzuki 2019-08-10 02:48:50 PDT
Created attachment 376007 [details]
Patch
Comment 6 Build Bot 2019-08-10 02:52:06 PDT
Attachment 376007 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Optional.h:585:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 3 in 195 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Yusuke Suzuki 2019-08-10 03:04:10 PDT
Created attachment 376008 [details]
Patch
Comment 8 Build Bot 2019-08-10 03:06:18 PDT
Attachment 376008 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Optional.h:585:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 3 in 193 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Build Bot 2019-08-10 05:38:52 PDT
Comment on attachment 376008 [details]
Patch

Attachment 376008 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12890469

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint
Comment 10 Yusuke Suzuki 2019-08-10 19:44:54 PDT
Created attachment 376026 [details]
Patch
Comment 11 Build Bot 2019-08-10 19:48:00 PDT
Attachment 376026 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Optional.h:585:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 3 in 194 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Build Bot 2019-08-10 22:02:17 PDT
Comment on attachment 376026 [details]
Patch

Attachment 376026 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12892898

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline
Comment 13 Saam Barati 2019-08-12 11:41:41 PDT
Comment on attachment 376026 [details]
Patch

rs=me
Comment 14 Mark Lam 2019-08-12 12:16:20 PDT
Comment on attachment 376026 [details]
Patch

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

> Source/WTF/wtf/DoublyLinkedList.h:69
> +class DoublyLinkedList {

I know we normally embed this in another class, but if any client chooses to malloc this, why not also make this FastMalloc'ed?
Comment 15 Yusuke Suzuki 2019-08-12 13:56:00 PDT
Comment on attachment 376026 [details]
Patch

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

>> Source/WTF/wtf/DoublyLinkedList.h:69
>> +class DoublyLinkedList {
> 
> I know we normally embed this in another class, but if any client chooses to malloc this, why not also make this FastMalloc'ed?

If we inherit this and RefCounted<T>, it poses ambiguous `new` definitions, and we need to put `WTF_MAKE_FAST_ALLOCATED` to the derived class again. And I do not wan to do that in this patch.
So this patch avoids annotating relatively-interface-like classes.
I'll later add makeUnique, which always has `static_assert` to check whether the class T is FastMalloced one.
I think this `makeUnique` and `RefCounted` can cover 100% of our use case, and both can ensure that all the classes are having FastMalloc annotation.
So, even if we do not annotate it with WTF_MAKE_FAST_ALLOCATED, later we still can find the classes that do not have WTF_MAKE_FAST_ALLOCATED by compile error.
Comment 16 Yusuke Suzuki 2019-08-12 13:57:28 PDT
Committed r248546: <https://trac.webkit.org/changeset/248546>
Comment 17 Radar WebKit Bug Importer 2019-08-12 13:58:20 PDT
<rdar://problem/54224528>
Comment 18 Darin Adler 2019-08-12 16:26:09 PDT
Here’s something else to consider for the future: If we create a WTF::makeUnique function, it does not need to check for WTF_MAKE_FAST_ALLOCATED. Instead it can have code that uses the fast allocator regardless of whether the class does WTF_MAKE_FAST_ALLOCATED. Rather than helping us sprinkle WTF_MAKE_FAST_ALLOCATED everywhere, it could reduce the importance of it. Then we would only need to use WTF_MAKE_FAST_ALLOCATED for classes and structs that need to use the fast allocator when *not* calling WTF::makeUnique.
Comment 19 Yusuke Suzuki 2019-08-12 23:20:48 PDT
(In reply to Darin Adler from comment #18)
> Here’s something else to consider for the future: If we create a
> WTF::makeUnique function, it does not need to check for
> WTF_MAKE_FAST_ALLOCATED. Instead it can have code that uses the fast
> allocator regardless of whether the class does WTF_MAKE_FAST_ALLOCATED.
> Rather than helping us sprinkle WTF_MAKE_FAST_ALLOCATED everywhere, it could
> reduce the importance of it. Then we would only need to use
> WTF_MAKE_FAST_ALLOCATED for classes and structs that need to use the fast
> allocator when *not* calling WTF::makeUnique.

Nice catch. After considering about this idea, personally, I like annotating classes with WTF_MAKE_FAST_ALLOCATED. Let me explain why I think so.

Since default deleter is performing `delete` and this is not what we want for FastMalloced ones, we need to return std::unique_ptr<T, FastFreeDeleter> for T if T does not have FastMalloc-annotation. To me, this sounds a bit dangerous. The dangerous example is the following.

auto pointer = WTF::makeUnique<T>();
// Super dangerous, but sometimes it is required...
auto* rawPointer = pointer.release();
// Passing rawPointer to somewhere, and
delete rawPointer;

The above one becomes invalid because pointer may start requiring non `delete` destroying function.
In the above case, the correct way becomes the following.

rawPointer->~T();
fastFree(rawPointer);

This looks non-intuitive. And having two ways to destroying objects (`delete` or the above one) can be error-prone.
If we have WTF_MAKE_FAST_ALLOCATED for T, we do not need to care about this. "new" and "delete" operators are defined, and C++ way works.
The simple invariant, "makeUnique just does `new` internally. And `delete` operator does `delete`. default deleter is just doing `delete`", is kept.


While we need to annotate many classes with WTF_MAKE_FAST_ALLOCATED, it is one time cost when we add a class.
And, by introducing `WTF::makeUnique<>`, we no longer forget adding this. So, personally I think just implementing `makeUnique` as follows looks simple :)

makeUnique(...)
    static_assert(T is FastMalloced or IsoHeaped);
    return make_unique<T>(...)
Comment 20 Yusuke Suzuki 2019-08-13 11:54:38 PDT
Committed r248603: <https://trac.webkit.org/changeset/248603>