Bug 153923 – REGRESSION (r193373): bmalloc: largeMax calculation is wrong on iOS

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

largeMax + sizeof(LargeChunk) > 1MB which is bad.

[Saam Barati]

Created attachment 270757 [details]
patch

[Mark Lam]

Comment on attachment 270757 [details]
patch

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

r=me with fixes.

> Source/bmalloc/ChangeLog:11
> +        space to actually allocate inside the LargeChunk. This made 
> +        it so that we tried to allocate a large object for something
> +        that really should be extra large.

Did you mean "This made it so that we will allocate a large object for something that should really be extra large."?

> Source/bmalloc/ChangeLog:14
> +        which meant that when we would grow() and allocate an

Remove "an"?

> Source/bmalloc/ChangeLog:16
> +        a new LargeObject, we wouldn't actually be able to fit
> +        in the object we were growing for into the new slot.

Did you mean "wouldn't actually be able to fit the object"?

> Source/bmalloc/bmalloc/LargeChunk.h:82
> +static_assert(largeChunkMetadataSize + largeMax <= largeChunkSize, "We will think we can accomidate larger objects than we can in reality if this condition does not hold");

typo: accomidate ==> accomodate.

[Michael Saboff]

Comment on attachment 270757 [details]
patch

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

> Source/bmalloc/bmalloc/Sizes.h:79
> +#if BPLATFORM(IOS)
> +    static const size_t largeChunkMetadataSize = 16 * kB;
> +#else
> +    static const size_t largeChunkMetadataSize = 4 * kB;
> +#endif

Can you change this to just be vmPageSize if that what the math should be?

[Saam Barati]

(In reply to comment #3)
> Comment on attachment 270757 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270757&action=review
> 
> > Source/bmalloc/bmalloc/Sizes.h:79
> > +#if BPLATFORM(IOS)
> > +    static const size_t largeChunkMetadataSize = 16 * kB;
> > +#else
> > +    static const size_t largeChunkMetadataSize = 4 * kB;
> > +#endif
> 
> Can you change this to just be vmPageSize if that what the math should be?

I was thinking that it lines up nicely, but it's not required
that they're the same, even though they're currently the same.
It just needs to be aligned on page boundaries. I think
it makes sense to keep the math as is as a better descriptor
of intention.

[Saam Barati]

We could add a static assert that the object is divisible by 
the system's page size.

[Geoffrey Garen]

Comment on attachment 270757 [details]
patch

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

> Source/bmalloc/bmalloc/LargeChunk.h:81
> +static_assert(largeChunkMetadataSize == sizeof(LargeChunk), "largeChunkMetadataSize should be the number as sizeof(LargeChunk) or our math in Size.h is wrong");

Sizes.h

You can be more specific and mention largeMax

>> Source/bmalloc/bmalloc/LargeChunk.h:82
>> +static_assert(largeChunkMetadataSize + largeMax <= largeChunkSize, "We will think we can accomidate larger objects than we can in reality if this condition does not hold");
> 
> typo: accomidate ==> accomodate.

I would remove "if this condition does not hold" -- all assertions are like that -- you only see the message because the condition didn't hold.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/196186

[David Kilzer (:ddkilzer)]

<rdar://problem/18620635>

[David Kilzer (:ddkilzer)]

Per internal discussion, this was a regression from the fix for Bug 151817.

Bug 151817: bmalloc: extra large allocations could be more efficient
<https://bugs.webkit.org/show_bug.cgi?id=151817>
<http://trac.webkit.org/changeset/193373>