largeMax + sizeof(LargeChunk) > 1MB which is bad.
Created attachment 270757 [details] patch
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.
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?
(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.
We could add a static assert that the object is divisible by the system's page size.
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.
landed in: http://trac.webkit.org/changeset/196186
<rdar://problem/18620635>
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>