Bug 208367

Summary: Add document about WTF malloc-related macros
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: Web Template FrameworkAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, clopez, cmarcelo, darin, ews-watchlist, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Description Yusuke Suzuki 2020-02-28 01:37:19 PST
Like, WTF_MAKE_FAST_ALLOCATED, WTF_MAKE_ISO_ALLOCATED etc.
Comment 1 Yusuke Suzuki 2020-04-20 16:10:43 PDT
Created attachment 397032 [details]
Patch
Comment 2 Yusuke Suzuki 2020-04-20 16:14:27 PDT
Created attachment 397033 [details]
Patch
Comment 3 Simon Fraser (smfr) 2020-04-20 16:21:59 PDT
Comment on attachment 397033 [details]
Patch

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

> Source/WTF/wtf/FastMalloc.h:31
> +// allocation is handled bmalloc if bmalloc is available.

handled by bmalloc

> Source/WTF/wtf/FastMalloc.h:35
> +//   1. If your class / struct is derived one of the base class using WTF_MAKE_ISO_ALLOCATED / WTF_MAKE_ISO_ALLOCATED_EXPORT,

derived from a base class which uses

> Source/WTF/wtf/FastMalloc.h:42
> +// Let's explain the difference in details.

Let's explain the differences in detail.

> Source/WTF/wtf/FastMalloc.h:46
> +//     If you annotate class / struct with these macros, class / struct is allocated with FastMalloc (bmalloc if it is available).

I don't think you need to say "If you annotate class / struct with these macros"

> Source/WTF/wtf/FastMalloc.h:49
> +//     WTF_MAKE_FAST_ALLOCATED is for class and WTF_MAKE_STRUCT_FAST_ALLOCATED is for struct. The difference between them are how we

WTF_MAKE_FAST_ALLOCATED is for classes and WTF_MAKE_STRUCT_FAST_ALLOCATED is for structs

> Source/WTF/wtf/FastMalloc.h:54
> +//     If you annotate class / struct with these macros, in release build, this is completely the same to WTF_MAKE_FAST_ALLOCATED etc.

Normally, these are identical to WTF_MAKE_FAST_ALLOCATED.

> Source/WTF/wtf/FastMalloc.h:55
> +//     These macros act differently when MallocHeapBreakdown debugging feature is enabled. When this feature is enabled, bmalloc is switched

When the MallocHeapBreakdown debugging feature

> Source/WTF/wtf/FastMalloc.h:56
> +//     to using System Malloc internally, and bmalloc creates MallocZone per this annotation. MallocZone allows us to track heap usage

creates a named MallocZone
Comment 4 Yusuke Suzuki 2020-04-20 16:28:06 PDT
Comment on attachment 397033 [details]
Patch

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

Thanks

>> Source/WTF/wtf/FastMalloc.h:31
>> +// allocation is handled bmalloc if bmalloc is available.
> 
> handled by bmalloc

Fixed.

>> Source/WTF/wtf/FastMalloc.h:35
>> +//   1. If your class / struct is derived one of the base class using WTF_MAKE_ISO_ALLOCATED / WTF_MAKE_ISO_ALLOCATED_EXPORT,
> 
> derived from a base class which uses

Fixed.

>> Source/WTF/wtf/FastMalloc.h:42
>> +// Let's explain the difference in details.
> 
> Let's explain the differences in detail.

Fixed.

>> Source/WTF/wtf/FastMalloc.h:46
>> +//     If you annotate class / struct with these macros, class / struct is allocated with FastMalloc (bmalloc if it is available).
> 
> I don't think you need to say "If you annotate class / struct with these macros"

Removed.

>> Source/WTF/wtf/FastMalloc.h:49
>> +//     WTF_MAKE_FAST_ALLOCATED is for class and WTF_MAKE_STRUCT_FAST_ALLOCATED is for struct. The difference between them are how we
> 
> WTF_MAKE_FAST_ALLOCATED is for classes and WTF_MAKE_STRUCT_FAST_ALLOCATED is for structs

Fixed.

>> Source/WTF/wtf/FastMalloc.h:54
>> +//     If you annotate class / struct with these macros, in release build, this is completely the same to WTF_MAKE_FAST_ALLOCATED etc.
> 
> Normally, these are identical to WTF_MAKE_FAST_ALLOCATED.

Fixed.

>> Source/WTF/wtf/FastMalloc.h:55
>> +//     These macros act differently when MallocHeapBreakdown debugging feature is enabled. When this feature is enabled, bmalloc is switched
> 
> When the MallocHeapBreakdown debugging feature

Fixed.

>> Source/WTF/wtf/FastMalloc.h:56
>> +//     to using System Malloc internally, and bmalloc creates MallocZone per this annotation. MallocZone allows us to track heap usage
> 
> creates a named MallocZone

Fixed.
Comment 5 Yusuke Suzuki 2020-04-20 16:29:43 PDT
Created attachment 397037 [details]
Patch
Comment 6 Darin Adler 2020-04-20 17:47:29 PDT
Comment on attachment 397037 [details]
Patch

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

I love the idea of having comments. The shorter and simpler this can be, the better. Already good.

> Source/WTF/wtf/FastMalloc.h:37
> +//   2. If your class / struct is JavaScript exposed one, use WTF_MAKE_ISO_ALLOCATED.

I don’t know what "JavaScript exposed" means. Maybe we could be more specific and mechanical on how we can tell if a class or struct is "JavaScript exposed"?

> Source/WTF/wtf/FastMalloc.h:47
> +//     Because of performance and memory footprint, we encourage using FastMalloc for all the class / struct allocations if possible.

The issue is that if we don’t use WTF_MAKE_FAST_ALLOCATED then new/delete will use operator new/delete, which will use system malloc. And we want to minimize the use of system malloc, because bmalloc offers better performance and memory footprint. But that’s not stated here.
Comment 7 Yusuke Suzuki 2020-04-20 19:45:02 PDT
Comment on attachment 397037 [details]
Patch

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

>> Source/WTF/wtf/FastMalloc.h:37
>> +//   2. If your class / struct is JavaScript exposed one, use WTF_MAKE_ISO_ALLOCATED.
> 
> I don’t know what "JavaScript exposed" means. Maybe we could be more specific and mechanical on how we can tell if a class or struct is "JavaScript exposed"?

I should rewrite it with `DOM object`, maybe this is much better to clarify this.

>> Source/WTF/wtf/FastMalloc.h:47
>> +//     Because of performance and memory footprint, we encourage using FastMalloc for all the class / struct allocations if possible.
> 
> The issue is that if we don’t use WTF_MAKE_FAST_ALLOCATED then new/delete will use operator new/delete, which will use system malloc. And we want to minimize the use of system malloc, because bmalloc offers better performance and memory footprint. But that’s not stated here.

Nice, I've noted about it.
Comment 8 Yusuke Suzuki 2020-04-20 19:53:00 PDT
Committed r260409: <https://trac.webkit.org/changeset/260409>
Comment 9 Radar WebKit Bug Importer 2020-04-20 19:53:39 PDT
<rdar://problem/62088206>