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

Yusuke Suzuki
Reported 2019-02-19 15:43:47 PST
...
Attachments
Patch (4.59 KB, patch)
2019-02-19 18:21 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2019-02-19 18:21:55 PST
Created attachment 362468 [details] Patch
Mark Lam
Comment 2 2019-02-20 10:05:09 PST
Comment on attachment 362468 [details] Patch r=me
Yusuke Suzuki
Comment 3 2019-02-20 13:45:59 PST
(In reply to Mark Lam from comment #2) > Comment on attachment 362468 [details] > Patch > > r=me Thanks!
WebKit Commit Bot
Comment 4 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>
WebKit Commit Bot
Comment 5 2019-02-20 14:22:19 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 6 2019-02-20 14:23:29 PST
<rdar://problem/48251809>
Darin Adler
Comment 7 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.
Yusuke Suzuki
Comment 8 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 :)
Note You need to log in before you can comment on or make changes to this bug.