[webkit-reviews] review denied: [Bug 64262] Small speed up, which switches some virtual functions to inline ones. : [Attachment 101148] Small speed up, which switches some virtual functions to inline ones.

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Czene Tamas
<Czene.Tamas at stud.u-szeged.hu>'s request for review:
Bug 64262: Small speed up, which switches some virtual functions to inline
ones.
https://bugs.webkit.org/show_bug.cgi?id=64262

Attachment 101148: Small speed up, which switches some virtual functions to
inline ones.
https://bugs.webkit.org/attachment.cgi?id=101148&action=review

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


You are changing from virtual function to protected attribute for 3 features:
::isFixedSize()
::isQuirkValue()
::isPrimitiveValue()

In each case, you are increasing complexity of the code, and increasing the
memory consumption per instance.

I would prefer to see 3 patches, one for each value. With numbers for
performance and memory usage (ideally 3 bugs on bugzilla).

When changing something used all over the place like CSSValue, I think it is
better to do small changes with a good rationale.

> Source/WebCore/css/CSSCanvasValue.h:52
> +	   m_isFixedSize = true;

This should use the constructor initialiazation: m_isFixedSize(true);

> Source/WebCore/css/CSSPrimitiveValue.h:210
> +	   m_isPrimitiveValue = true;

Initialization...

> Source/WebCore/css/CSSPrimitiveValue.h:220
> +	   m_isPrimitiveValue = true;

Initialization...

> Source/WebCore/css/CSSPrimitiveValue.h:229
> +	   m_isPrimitiveValue = true;

Initialization...

> Source/WebCore/css/CSSPrimitiveValue.h:243
> +	   m_isPrimitiveValue = true;

Initialization...

> Source/WebCore/css/CSSPrimitiveValue.h:251
> +	   m_isPrimitiveValue = true;

Initialization...

> Source/WebCore/css/CSSQuirkPrimitiveValue.h:43
> +	   m_isQuirkValue = true;

Constructor initialization: m_isQuirkValue(true);