Bug 186322 - Switch to system malloc on iOS when nano malloc is disabled
Summary: Switch to system malloc on iOS when nano malloc is disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-05 14:18 PDT by Saam Barati
Modified: 2018-06-26 12:40 PDT (History)
16 users (show)

See Also:


Attachments
patch (5.21 KB, patch)
2018-06-25 18:45 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-06-05 14:18:59 PDT
...
Comment 1 Geoffrey Garen 2018-06-05 14:35:02 PDT
I don't think we should do this.
Comment 2 Filip Pizlo 2018-06-05 16:47:57 PDT
(In reply to Geoffrey Garen from comment #1)
> I don't think we should do this.

We have data that it's either a space progression or is space-neutral, and that it's time-neutral.  So, I think we should do it.
Comment 3 Radar WebKit Bug Importer 2018-06-07 16:08:22 PDT
<rdar://problem/40914671>
Comment 4 Saam Barati 2018-06-25 18:45:34 PDT
Created attachment 343572 [details]
patch
Comment 5 EWS Watchlist 2018-06-25 18:48:28 PDT
Attachment 343572 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Environment.cpp:42:  malloc_engaged_nano is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Saam Barati 2018-06-25 19:28:50 PDT
Comment on attachment 343572 [details]
patch

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

> Source/bmalloc/bmalloc/BPlatform.h:241
> +#if ((BPLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000) || (BPLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 50000) || (BPLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 120000)) \

I can simplify this and just make this IOS since the other two are true if this is IOS
Comment 7 Keith Miller 2018-06-25 21:34:31 PDT
Comment on attachment 343572 [details]
patch

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

r=me.

>> Source/bmalloc/bmalloc/BPlatform.h:241
>> +#if ((BPLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000) || (BPLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 50000) || (BPLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 120000)) \
> 
> I can simplify this and just make this IOS since the other two are true if this is IOS

I'm confused is __TV_OS_VERSION_MIN_REQUIRED defined on iOS? Or are you saying that if __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000 then __TV_OS_VERSION_MIN_REQUIRED will be >= 120000 also?

> Source/bmalloc/bmalloc/ProcessCheck.mm:62
> +        if (NSString *appName = [[NSBundle mainBundle] bundleIdentifier]) {

Maybe we should look for an environment variable too?
Comment 8 Saam Barati 2018-06-25 23:16:18 PDT
Comment on attachment 343572 [details]
patch

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

>>> Source/bmalloc/bmalloc/BPlatform.h:241
>>> +#if ((BPLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000) || (BPLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 50000) || (BPLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 120000)) \
>> 
>> I can simplify this and just make this IOS since the other two are true if this is IOS
> 
> I'm confused is __TV_OS_VERSION_MIN_REQUIRED defined on iOS? Or are you saying that if __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000 then __TV_OS_VERSION_MIN_REQUIRED will be >= 120000 also?

I realized this is actually required. I'm not going to simplify this.

Keith, I'm not sure what you're question is here, __TV_OS_VERSION_MIN_REQUIRED is 120000 for the newest tvOS version

>> Source/bmalloc/bmalloc/ProcessCheck.mm:62
>> +        if (NSString *appName = [[NSBundle mainBundle] bundleIdentifier]) {
> 
> Maybe we should look for an environment variable too?

Seems unnecessary ATM since we have malloc environment variables. We should do that in the future if we use this for more things that controlling system malloc.
Comment 9 WebKit Commit Bot 2018-06-26 00:38:13 PDT
Comment on attachment 343572 [details]
patch

Clearing flags on attachment: 343572

Committed r233192: <https://trac.webkit.org/changeset/233192>
Comment 10 WebKit Commit Bot 2018-06-26 00:38:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Simon Fraser (smfr) 2018-06-26 10:28:46 PDT
Comment on attachment 343572 [details]
patch

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

> Source/bmalloc/ChangeLog:3
> +        Wasm: Any function argument of type Void should be a validation error

This seems like the wrong title
Comment 12 Saam Barati 2018-06-26 12:15:52 PDT
Comment on attachment 343572 [details]
patch

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

>> Source/bmalloc/ChangeLog:3
>> +        Wasm: Any function argument of type Void should be a validation error
> 
> This seems like the wrong title

Ugh, yeah, I used the wrong bug ID in prepare-ChangeLog
Comment 13 Saam Barati 2018-06-26 12:40:07 PDT
Committed: https://trac.webkit.org/changeset/233216/webkit
- Fixes watchOS build
- Fixes bad changelog title and bug id