[webkit-reviews] review denied: [Bug 87078] Make CSSParser::filteredProperties() O(n) instead of O(n^2) and improve readability. : [Attachment 143423] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 87078: Make CSSParser::filteredProperties() O(n) instead of O(n^2) and
improve readability.
https://bugs.webkit.org/show_bug.cgi?id=87078

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=143423&action=review


Do you have some real world benchmark that you are trying to improve? Otherwise
this seems bit like changing code for the sake of changing code.

> Source/WebCore/ChangeLog:9
> +	   1) Make the code more linearly readable by separating out handling
of important and non-important properties.

I don't think it is any more readable.

> Source/WebCore/ChangeLog:10
> +	   2) Eliminate one BitArray instance (reduces hot memory so more cache
friendly).

This is unlikely to have any real effect considering how small BitArrays here
are (48bytes).

> Source/WebCore/ChangeLog:11
> +	   3) Use unchecked array appends (checking is not necessary because
capacity has already been reserved).

This makes sense as a tiny micro-optimization.

> Source/WebCore/ChangeLog:14
> +	   4) Remove O(n^2) behavior caused by scanning for and removing
previously encountered definitions of each property.
> +	      This was quite bad because we would remove early in the vector
causing a move of all subsequent entries, then
> +	      add at the end to keep the order correct.

The O(n^2) here is an artificial edge case (the same property repeated many
times in a single rule).

> Source/WebCore/css/CSSParser.cpp:1133
> +    // Add important properties in reverse order, skipping already set
properties.
> +    for (int i = m_parsedProperties.size() - 1; i >= 0; --i) {

!important properties are rare. This adds unnecessary loop for the common case.