Bug 156593 – AffectsNextSibling style relation marking is inefficient

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

[Antti Koivisto]

We add a StyleRelation entry for each sibling to mark. With long sibling lists this can be inefficient in terms of both memory and speed. Instead make a single entry that includes the sibling count to mark.

[Antti Koivisto]

Created attachment 276420 [details]
patch

[Antti Koivisto]

Created attachment 276426 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/25735554>

[Benjamin Poulain]

Comment on attachment 276426 [details]
patch

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

> Source/WebCore/css/SelectorChecker.cpp:94
> +        if (last.type == Style::Relation::AffectsNextSibling && last.element->previousElementSibling() == &element) {

It would likely be better to do:
    element ->nextElementSibling() == last.element
to avoid touching the memory of last.element again.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2214
> +        LocalRegister lastRelation(m_registerAllocator);
> +        m_assembler.load32(sizeAddress, lastRelation);

It would be better to have this load above the mergeFailure above.

Currently the CPU will have to do two loads:
1) m_assembler.branchTest32(Assembler::Zero, sizeAddress)
2) m_assembler.load32(sizeAddress, lastRelation);

If you write:
LocalRegister size(m_registerAllocator);
m_assembler.load32(sizeAddress, size);
mergeFailure.append(m_assembler.branchTest32(Assembler::Zero, size));
m_assembler.sub32(Assembler::TrustedImm32(1), size);
...

Then you only need one load.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2237
> +        m_assembler.load32(valueAddress, value);
> +        m_assembler.add32(Assembler::TrustedImm32(1), value);
> +        m_assembler.store32(value, valueAddress);

If you want you can do 
    m_assembler.add32(Assembler::TrustedImm32(1), valueAddress);

That's one instruction on x86. On ARM it generates the same code as you wrote.

[Antti Koivisto]

Created attachment 276464 [details]
patch

[Antti Koivisto]

> It would likely be better to do:
>     element ->nextElementSibling() == last.element
> to avoid touching the memory of last.element again.

Good idea. Did this also on the compiler path.

[Antti Koivisto]

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