Bug 108410 – Vector should consult allocator about ideal size when choosing capacity.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Andreas Kling]

Vector should consult allocator about ideal size when choosing capacity.

[Andreas Kling]

Created attachment 185640 [details]
Patch

[Benjamin Poulain]

Comment on attachment 185640 [details]
Patch

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

> Source/WTF/wtf/Vector.h:255
> +            newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T);

Why not also tryAllocateBuffer() and reallocateBuffer()?

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 185640 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185640&action=review
> 
> > Source/WTF/wtf/Vector.h:255
> > +            newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T);
> 
> Why not also tryAllocateBuffer() and reallocateBuffer()?

Because blind spot. :(

[Andreas Kling]

Created attachment 185645 [details]
Patch

Kinda curious how EWS likes this.

[Benjamin Poulain]

Comment on attachment 185645 [details]
Patch

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

I love this patch.

Overflow?

> Source/JavaScriptCore/ChangeLog:8
> +        Remove assertion about Vector capacity that won't hold anymore since.

"that won't hold anymore since."?

> Source/WTF/wtf/Deque.h:387
> -        size_t newCapacity = std::max(static_cast<size_t>(16), oldCapacity + oldCapacity / 4 + 1);
>          T* oldBuffer = m_buffer.buffer();
> -        m_buffer.allocateBuffer(newCapacity);
> +        m_buffer.allocateBuffer(std::max(static_cast<size_t>(16), oldCapacity + oldCapacity / 4 + 1));

I liked the variable newCapacity :)

[Andreas Kling]

Created attachment 185652 [details]
Patch

So much thrashing. I blame the coffee.

@Benjamin: I removed the 'newCapacity' local in Deque since I have to read back m_buffer.capacity() for the correct value.

[Benjamin Poulain]

Comment on attachment 185652 [details]
Patch

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

> Source/WTF/wtf/Vector.h:259
> +            newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T);
> +            m_capacity = newCapacity;
>              m_buffer = static_cast<T*>(fastMalloc(newCapacity * sizeof(T)));

Okay, now, what about we can remove the /sizeof(T)? :-D

size_t sizeToAllocate = fastMallocGoodSize(newCapacity * sizeof(T));
m_capacity = sizeToAllocate / sizeof(T)
m_buffer = mallo(sizeToAllocate).

> Source/WTF/wtf/Vector.h:271
> +            newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T);
>              T* newBuffer;
>              if (tryFastMalloc(newCapacity * sizeof(T)).getValue(newBuffer)) {
>                  m_capacity = newCapacity;

ditto

> Source/WTF/wtf/Vector.h:290
>              ASSERT(shouldReallocateBuffer(newCapacity));
> -            m_capacity = newCapacity;
>              if (newCapacity > std::numeric_limits<size_t>::max() / sizeof(T))
>                  CRASH();
> +            newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T);
> +            m_capacity = newCapacity;
>              m_buffer = static_cast<T*>(fastRealloc(m_buffer, newCapacity * sizeof(T)));

ditto.

[Andreas Kling]

(In reply to comment #7)
> (From update of attachment 185652 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185652&action=review
> 
> > Source/WTF/wtf/Vector.h:259
> > +            newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T);
> > +            m_capacity = newCapacity;
> >              m_buffer = static_cast<T*>(fastMalloc(newCapacity * sizeof(T)));
> 
> Okay, now, what about we can remove the /sizeof(T)? :-D
> 
> size_t sizeToAllocate = fastMallocGoodSize(newCapacity * sizeof(T));
> m_capacity = sizeToAllocate / sizeof(T)
> m_buffer = mallo(sizeToAllocate).
> 
> > Source/WTF/wtf/Vector.h:271
> > +            newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T);
> >              T* newBuffer;
> >              if (tryFastMalloc(newCapacity * sizeof(T)).getValue(newBuffer)) {
> >                  m_capacity = newCapacity;
> 
> ditto
> 
> > Source/WTF/wtf/Vector.h:290
> >              ASSERT(shouldReallocateBuffer(newCapacity));
> > -            m_capacity = newCapacity;
> >              if (newCapacity > std::numeric_limits<size_t>::max() / sizeof(T))
> >                  CRASH();
> > +            newCapacity = fastMallocGoodSize(newCapacity * sizeof(T)) / sizeof(T);
> > +            m_capacity = newCapacity;
> >              m_buffer = static_cast<T*>(fastRealloc(m_buffer, newCapacity * sizeof(T)));
> 
> ditto.

Sure, when I'm awake again. No need to reupload for that :)

[Build Bot]

Comment on attachment 185652 [details]
Patch

Attachment 185652 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16271120

[Radar WebKit Bug Importer]

<rdar://problem/13124002>

[Andreas Kling]

Committed r141407: <http://trac.webkit.org/changeset/141407>

[Andreas Kling]

(In reply to comment #9)
> (From update of attachment 185652 [details])
> Attachment 185652 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/16271120

Landed with Benjamin's comments addressed. This ews failure looks like a rebuild of QTMovieWin is needed.

[Jessie Berlin]

I had to roll this patch out in http://trac.webkit.org/changeset/141504 because it was causing crashes in release builds.

[Andreas Kling]

Created attachment 186239 [details]
Patch

Finally tracked down the crash, it was due to fastMallocGoodSize() getting called before the TCMalloc page heap was fully initialized.
Fixed this by calling TCMalloc_ThreadCache::InitModule() in fastMallocGoodSize() if needed.

[Benjamin Poulain]

Comment on attachment 186239 [details]
Patch

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

> Source/WTF/wtf/FastMalloc.cpp:2574
> +    if (!phinited)
> +        TCMalloc_ThreadCache::InitModule();
> +    return AllocationSize(bytes);
> +}

Any reason we couldn't generate class_to_size and class_to_pages  at compile time?

[Build Bot]

Comment on attachment 186239 [details]
Patch

Attachment 186239 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16358080

[Andreas Kling]

Committed r141716: <http://trac.webkit.org/changeset/141716>

[Andreas Kling]

(In reply to comment #15)
> (From update of attachment 186239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186239&action=review
> 
> > Source/WTF/wtf/FastMalloc.cpp:2574
> > +    if (!phinited)
> > +        TCMalloc_ThreadCache::InitModule();
> > +    return AllocationSize(bytes);
> > +}
> 
> Any reason we couldn't generate class_to_size and class_to_pages  at compile time?

We might be able to do that. Let's take that in a separate patch though.

[Darin Adler]

I made an attempt at this for String back in 2008 when strings were resizable. I guess there’s a chance that StringBuilder might benefit from this, even today.