Bug 194837

Summary: [bmalloc] DebugHeap::malloc does not have "try" version.
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, mark.lam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Yusuke Suzuki 2019-02-19 15:43:47 PST
...
Comment 1 Yusuke Suzuki 2019-02-19 18:21:55 PST
Created attachment 362468 [details]
Patch
Comment 2 Mark Lam 2019-02-20 10:05:09 PST
Comment on attachment 362468 [details]
Patch

r=me
Comment 3 Yusuke Suzuki 2019-02-20 13:45:59 PST
(In reply to Mark Lam from comment #2)
> Comment on attachment 362468 [details]
> Patch
> 
> r=me

Thanks!
Comment 4 WebKit Commit Bot 2019-02-20 14:22:18 PST
Comment on attachment 362468 [details]
Patch

Clearing flags on attachment: 362468

Committed r241837: <https://trac.webkit.org/changeset/241837>
Comment 5 WebKit Commit Bot 2019-02-20 14:22:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2019-02-20 14:23:29 PST
<rdar://problem/48251809>
Comment 7 Darin Adler 2019-02-20 21:51:11 PST
Why not put the crash on failure at the call sites? Seems strange to pass a boolean just to do a check that the caller could have done.
Comment 8 Yusuke Suzuki 2019-02-21 02:14:51 PST
(In reply to Darin Adler from comment #7)
> Why not put the crash on failure at the call sites? Seems strange to pass a
> boolean just to do a check that the caller could have done.

The patch adds missing tryAllocate feature into DebugHeap, and this style is aligned to the existing DebugHeap::memalign, bmalloc allocators, and JSC GC Heap Allocators (JavaScriptCore/heap/AllocationFailureMode.h).
I think our (current) typical style is, creating low-level allocator with this style, and constructing tryAllocate / allocate APIs on these low-level allocators, and expose them to clients (e.g. JSArray::tryCreate).
I'm not sure the original intent of this API design. But one pros is that we can gather CRASH into one place instead of putting it at the caller sites, while we still keep good "try" or "not-try" information in stacktrace's function name (`tryAllocate` or `allocate`).
But I don't have strong opinion here as long as the style is consistent :)