Bug 134416 – reduce dynamic memory allocation in css jit compiler

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alex Christensen]

The css jit compiler could be sped up.  Calls to malloc are slow.  Let's get rid of them.

[Alex Christensen]

Created attachment 234023 [details]
Patch

[WebKit Commit Bot]

Attachment 234023 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Deque.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 234023 [details]
Patch

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

> Source/WTF/ChangeLog:35
> +        (WTF::inlineCapacity>::checkValidity):
> +        (WTF::inlineCapacity>::checkIndexValidity):
> +        (WTF::inlineCapacity>::invalidateIterators):
> +        (WTF::inlineCapacity>::Deque):
> +        (WTF::=):
> +        (WTF::inlineCapacity>::destroyAll):
> +        (WTF::inlineCapacity>::~Deque):
> +        (WTF::inlineCapacity>::swap):
> +        (WTF::inlineCapacity>::clear):
> +        (WTF::inlineCapacity>::expandCapacityIfNeeded):
> +        (WTF::inlineCapacity>::expandCapacity):
> +        (WTF::inlineCapacity>::append):
> +        (WTF::inlineCapacity>::prepend):
> +        (WTF::inlineCapacity>::removeFirst):
> +        (WTF::inlineCapacity>::removeLast):
> +        (WTF::inlineCapacity>::remove):
> +        (WTF::inlineCapacity>::addToIteratorsList):
> +        (WTF::inlineCapacity>::removeFromIteratorsList):
> +        (WTF::inlineCapacity>::DequeIteratorBase):
> +        (WTF::inlineCapacity>::~DequeIteratorBase):
> +        (WTF::inlineCapacity>::isEqual):
> +        (WTF::inlineCapacity>::increment):
> +        (WTF::inlineCapacity>::decrement):
> +        (WTF::inlineCapacity>::after):
> +        (WTF::inlineCapacity>::before):

The script generated garbage here. Please either fix these manually or remove them. I think it’s OK to omit the list of function names entirely.

> Source/WebCore/cssjit/RegisterAllocator.h:65
> +    JSC::ARMRegisters::r9,

Change not mentioned in the change log.

> Source/WebCore/cssjit/RegisterAllocator.h:73
> +    JSC::ARMRegisters::r8,
> +    JSC::ARMRegisters::r10,
> +    JSC::ARMRegisters::r11,

Change not mentioned in the change log.

[Benjamin Poulain]

Comment on attachment 234023 [details]
Patch

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

+1 from me too. One comment:

> Source/WebCore/cssjit/StackAllocator.h:75
> -        unsigned registerCount = registerIDs.size();
> -        Vector<StackReference> stackReferences;
> -        stackReferences.reserveInitialCapacity(registerCount);
> +        Vector<StackReference, registerCount> stackReferences;

The stack allocation is not useful here because the vector is returned by value. This will have to be heap allocated in the copy constructor when returning.

[Alex Christensen]

Created attachment 234081 [details]
Patch

[Alex Christensen]

I'll put the additional armv7 registers in a different patch because that change was unrelated.

I expanded my usage of vector inline capacity throughout the selector compiler using 32 as a large number that will probably be more than the number of function calls, fragments, etc.

[Geoffrey Garen]

> +1 from me too. One comment:
> 
> > Source/WebCore/cssjit/StackAllocator.h:75
> > -        unsigned registerCount = registerIDs.size();
> > -        Vector<StackReference> stackReferences;
> > -        stackReferences.reserveInitialCapacity(registerCount);
> > +        Vector<StackReference, registerCount> stackReferences;
> 
> The stack allocation is not useful here because the vector is returned by value. This will have to be heap allocated in the copy constructor when returning.

I don't think so. Vector with inline capacity has an optimization to copy to inline capacity without malloc-ating. It's a little far removed from the Vector class itself, but here is the code:

    VectorBuffer(size_t capacity, size_t size = 0)
        : Base(inlineBuffer(), inlineCapacity, size)
    {
        if (capacity > inlineCapacity)
            Base::allocateBuffer(capacity);
    }

[Alex Christensen]

Created attachment 234083 [details]
Patch

[WebKit Commit Bot]

Attachment 234083 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Deque.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 234083 [details]
Patch

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.

[Alex Christensen]

http://trac.webkit.org/changeset/170605

Passing a Vector& to push matched pop and it avoided an appendVector.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 134484

[Benjamin Poulain]

I got into several crashes with this backtrace:
  * frame #0: 0x0000000100eb81c8 JavaScriptCore`WTFCrash + 72
    frame #1: 0x0000000101db1318 WebCore`WebCore::StackAllocator::pop(WTF::Vector<WebCore::StackAllocator::StackReference, 0ul, WTF::CrashOnOverflow> const&, WTF::Vector<JSC::ARM64Registers::RegisterID, 0ul, WTF::CrashOnOverflow> const&) + 432
    frame #2: 0x0000000101db54bc WebCore`WebCore::FunctionCall::restoreAllocatedCallerSavedRegisters() + 208
    frame #3: 0x0000000101da8c04 WebCore`WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementFunctionCallTest(JSC::AbstractMacroAssembler<JSC::ARM64Assembler>::JumpList&, JSC::FunctionPtr) + 240
    frame #4: 0x0000000101da6170 WebCore`WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementMatching(JSC::AbstractMacroAssembler<JSC::ARM64Assembler>::JumpList&, JSC::AbstractMacroAssembler<JSC::ARM64Assembler>::JumpList&, WebCore::SelectorCompiler::SelectorFragment const&) + 616
    frame #5: 0x0000000101da56d4 WebCore`WebCore::SelectorCompiler::SelectorCodeGenerator::generateSelectorChecker() + 936
    frame #6: 0x0000000101db04ac WebCore`WebCore::SelectorCompiler::SelectorCodeGenerator::compile(JSC::VM*, JSC::MacroAssemblerCodeRef&) + 72
    frame #7: 0x0000000101da5300 WebCore`WebCore::SelectorCompiler::compileSelector(WebCore::CSSSelector const*, JSC::VM*, WebCore::SelectorCompiler::SelectorContext, JSC::MacroAssemblerCodeRef&) + 84
    frame #8: 0x0000000101dbc9c0 WebCore`void WebCore::SelectorDataList::execute<WebCore::AllElementExtractorSelectorQueryTrait>(WebCore::ContainerNode&, WebCore::AllElementExtractorSelectorQueryTrait::OutputType&) const + 2496
    frame #9: 0x0000000101dbb988 WebCore`WebCore::SelectorDataList::queryAll(WebCore::ContainerNode&) const + 32
    frame #10: 0x00000001013056fc WebCore`WebCore::ContainerNode::querySelectorAll(WTF::String const&, int&) + 52
    frame #11: 0x0000000101888988 WebCore`WebCore::jsElementPrototypeFunctionQuerySelectorAll(JSC::ExecState*) + 340

It's odd because the patch had nothing to do with ARMv8. We'll need to look into that.

[Alex Christensen]

I see what it was.  The very last line should not have changed registerIDs.size() to registerCount.  I don't know why I did that :(

[Alex Christensen]

Created attachment 234181 [details]
Patch

[WebKit Commit Bot]

Attachment 234181 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Deque.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

(In reply to comment #14)
> I see what it was.  The very last line should not have changed registerIDs.size() to registerCount.  I don't know why I did that :(
The problem was the exact opposite.  I should have changed all registerCount to registerIDs.size().  I added new variable names to avoid confusion between registerCount and popRegisterCount or pushRegisterCount.

[Alex Christensen]

http://trac.webkit.org/changeset/170697