[webkit-reviews] review granted: [Bug 134416] reduce dynamic memory allocation in css jit compiler : [Attachment 234083] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 134416: reduce dynamic memory allocation in css jit compiler
https://bugs.webkit.org/show_bug.cgi?id=134416

Attachment 234083: Patch
https://bugs.webkit.org/attachment.cgi?id=234083&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=234083&action=review


Some comments, nothing critical.

> Source/WebCore/cssjit/FunctionCall.h:183
> +    Vector<std::pair<JSC::MacroAssembler::Call, JSC::FunctionPtr>, 32>&
m_callRegistry;

You can typedef this to avoid repeating the 32.

> Source/WebCore/cssjit/SelectorCompiler.cpp:166
> -    Vector<const AtomicStringImpl*, 1> classNames;
> +    Vector<const AtomicStringImpl*, 32> classNames;
>      HashSet<unsigned> pseudoClasses;
> -    Vector<JSC::FunctionPtr> unoptimizedPseudoClasses;
> -    Vector<AttributeMatchingInfo> attributes;
> -    Vector<std::pair<int, int>> nthChildFilters;
> +    Vector<JSC::FunctionPtr, 32> unoptimizedPseudoClasses;
> +    Vector<AttributeMatchingInfo, 32> attributes;
> +    Vector<std::pair<int, int>, 32> nthChildFilters;

I am not sure that is a good idea. The filters unoptimizedPseudoClasses and
nthChildFilters are uncommon. The attributes rarely goes over 1. Classe names
are usually 1 or 2.

> Source/WebCore/cssjit/SelectorCompiler.cpp:196
> +typedef Vector<SelectorFragment, 32> SelectorFragmentList;
> +typedef Vector<TagNamePattern, 32> TagNameList;

Again, this seems a little big.

> Source/WebCore/cssjit/StackAllocator.h:75
> +    Vector<StackReference, registerCount> push(const
Vector<JSC::MacroAssembler::RegisterID>& registerIDs)
>      {
>	   RELEASE_ASSERT(!m_hasFunctionCallPadding);
> -	   unsigned registerCount = registerIDs.size();
> -	   Vector<StackReference> stackReferences;
> -	   stackReferences.reserveInitialCapacity(registerCount);
> +	   Vector<StackReference, registerCount> stackReferences;

I think you need to get the output as an argument to avoid the copy.