Bug 234091 – Reduce maximum mmap size for Structure regions to help placate ios

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Reduce maximum mmap size for Structure regions to help placate ios

[Keith Miller]

Created attachment 446568 [details]
Patch

[Keith Miller]

Created attachment 446571 [details]
Patch

[Saam Barati]

Comment on attachment 446568 [details]
Patch

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

r=me

> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:93
> +    void* aligned = nullptr;

let's call this "result" or similar, maybe "alignedResult".

> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:95
> +    bool copy = false;
> +    int flags = VM_FLAGS_ANYWHERE;

constexpr

> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:97
> +    kern_return_t result = mach_vm_map(mach_task_self(), reinterpret_cast<mach_vm_address_t*>(&aligned), bytes, bytes - 1, flags, MEMORY_OBJECT_NULL, 0, copy, protections, protections, childProcessInheritance);

bitwise_cast

Also, this code is wrong if not a power of 2, right?

Should we assert we're dealing with a power of 2?

> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:104
> +#if HAVE(MADV_FREE_REUSE)
> +    if (aligned) {
> +        // To support the "reserve then commit" model, we have to initially decommit.
> +        while (madvise(aligned, bytes, MADV_FREE_REUSABLE) == -1 && errno == EAGAIN) { }
> +    }
> +#endif

Can we abstract this since we have a similar loop elsewhere?

> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:106
> +    return reinterpret_cast<void*>(aligned);

bitwise_cast

[Saam Barati]

Comment on attachment 446571 [details]
Patch

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

r=me still with comment from prior patch

> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:83
> +    UNUSED_PARAM(includesGuardPages);

should we assert we're not dealing with this?

[Keith Miller]

Comment on attachment 446568 [details]
Patch

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

>> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:93
>> +    void* aligned = nullptr;
> 
> let's call this "result" or similar, maybe "alignedResult".

I'll rename it to address.

>> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:95
>> +    int flags = VM_FLAGS_ANYWHERE;
> 
> constexpr

sure.

>> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:97
>> +    kern_return_t result = mach_vm_map(mach_task_self(), reinterpret_cast<mach_vm_address_t*>(&aligned), bytes, bytes - 1, flags, MEMORY_OBJECT_NULL, 0, copy, protections, protections, childProcessInheritance);
> 
> bitwise_cast
> 
> Also, this code is wrong if not a power of 2, right?
> 
> Should we assert we're dealing with a power of 2?

Yeah the assert at the top of the function checks that and that the bytes is bigger than page size.

>> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:104
>> +#endif
> 
> Can we abstract this since we have a similar loop elsewhere?

That seems like more code than we save.

>> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:106
>> +    return reinterpret_cast<void*>(aligned);
> 
> bitwise_cast

I can actually delete this.

[Keith Miller]

Created attachment 446574 [details]
Patch for landing

[EWS]

Patch 446574 does not build

[Keith Miller]

Created attachment 446589 [details]
Patch for landing

[Keith Miller]

Created attachment 446594 [details]
Patch for landing

[Keith Miller]

Created attachment 446601 [details]
Patch for landing

[Saam Barati]

(In reply to Keith Miller from comment #5)
> Comment on attachment 446568 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446568&action=review
> 
> >> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:93
> >> +    void* aligned = nullptr;
> > 
> > let's call this "result" or similar, maybe "alignedResult".
> 
> I'll rename it to address.
> 
> >> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:95
> >> +    int flags = VM_FLAGS_ANYWHERE;
> > 
> > constexpr
> 
> sure.
> 
> >> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:97
> >> +    kern_return_t result = mach_vm_map(mach_task_self(), reinterpret_cast<mach_vm_address_t*>(&aligned), bytes, bytes - 1, flags, MEMORY_OBJECT_NULL, 0, copy, protections, protections, childProcessInheritance);
> > 
> > bitwise_cast
> > 
> > Also, this code is wrong if not a power of 2, right?
> > 
> > Should we assert we're dealing with a power of 2?
> 
> Yeah the assert at the top of the function checks that and that the bytes is
> bigger than page size.
> 
> >> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:104
> >> +#endif
> > 
> > Can we abstract this since we have a similar loop elsewhere?
> 
> That seems like more code than we save.
> 
> >> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:106
> >> +    return reinterpret_cast<void*>(aligned);
> > 
> > bitwise_cast
> 
> I can actually delete this.

Not all of these changes were made.

[Saam Barati]

Comment on attachment 446568 [details]
Patch

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

>>> Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp:104
>>> +#endif
>> 
>> Can we abstract this since we have a similar loop elsewhere?
> 
> That seems like more code than we save.

that's not always the reason to abstract stuff like this. It's nice to have a shared implementation IMO because of the looping we do.

[EWS]

Committed r286804 (245044@main): <https://commits.webkit.org/245044@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446601 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86292683>

[Ryan Haddad]

Reverted r286804 for reason:

Breaks internal builds.

Committed r286818 (245054@trunk): <https://commits.webkit.org/245054@trunk>

[Keith Miller]

Created attachment 446714 [details]
Patch for landing

[EWS]

Committed r286849 (245082@main): <https://commits.webkit.org/245082@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446714 [details].