[webkit-reviews] review denied: [Bug 61025] Speed up SVGSMILElement::findInstanceTime : [Attachment 98707] Speed up SVGSMILElement::findInstanceTime

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 27 08:33:53 PDT 2011


Nikolas Zimmermann <zimmermann at kde.org> has denied Oliver Varga
<Varga.Oliver at stud.u-szeged.hu>'s request for review:
Bug 61025: Speed up SVGSMILElement::findInstanceTime
https://bugs.webkit.org/show_bug.cgi?id=61025

Attachment 98707: Speed up SVGSMILElement::findInstanceTime
https://bugs.webkit.org/attachment.cgi?id=98707&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98707&action=review

Almost there, still some tweaks to do, but much better than the last one!

> Source/JavaScriptCore/wtf/StdLibExtras.h:128
>  // Optimized for cases where the array contains the key, checked by
assertions.

This comment is not true anymore.

> Source/JavaScriptCore/wtf/StdLibExtras.h:130
> +inline ArrayType* binarySearch(ArrayType* array, size_t size, KeyType key,
bool isElementContains)

Please add sth like:
enum BinarySearchMode {
    KeyMustBePresentInArray,
    KeyMustNotBePresentInArray
};
and switch from bool isElementContains, to "BinarySearchMode mode =
KeyMustBePresentInArray".
This way you don't need to touch any other place that used binarySearch before,
and it's cleaner than a boolean param.

> Source/JavaScriptCore/wtf/StdLibExtras.h:133
>  {
>      // The array must contain at least one element (pre-condition, array
does conatin key).
>      // If the array only contains one element, no need to do the comparison.


This can be rephrased as well, and you can fix the existing conatin typo.

> Source/JavaScriptCore/wtf/StdLibExtras.h:152
>	   // 'size' should never reach zero.

Comment is also not true anymore for isElementContains=false.

> Source/JavaScriptCore/wtf/StdLibExtras.h:157
>      // If we reach this point we've chopped down to one element, no need to
check it matches

Ditto.

> Source/JavaScriptCore/wtf/StdLibExtras.h:162
> +    if (isElementContains)
> +	   ASSERT(size == 1);
> +
> +    if (isElementContains)
> +	   ASSERT(key == extractKey(&array[0]));

You can combine these if branches into one.

> Source/WebCore/ChangeLog:11
> +	   No new tests this is only a	performance tweak.

one space too much.

> Source/WebCore/svg/animation/SVGSMILElement.cpp:620
> +{

Style, must be "* position".

> Source/WebCore/svg/animation/SVGSMILElement.cpp:633
> +    int indexOfResult = (((unsigned)result) - ((unsigned)list.begin())) /
sizeof(SMILTime);

C-style casts shouldn't be used. Why do you need these casts? The type is also
wrong, if you cast then to ptrdiff_t, but I think it's unncessary.

> Source/WebCore/svg/animation/SVGSMILElement.cpp:636
> +	   indexOfResult++;

Use postfix ++indexResult.

> Source/WebCore/svg/animation/SVGSMILElement.cpp:638
> +    if (list[indexOfResult].isIndefinite() && beginOrEnd == Begin)

You should add an assert before this line: ASSERT(indexOfResult < sizeOfList).

> Source/WebCore/svg/animation/SVGSMILElement.cpp:639
> +	   // "The special value "indefinite" does not yield an instance time
in the begin list."

You can move the comment before the if condition, as its our common style.

> Source/WebCore/svg/animation/SVGSMILElement.cpp:652
> +	   indexOfResult++;

Use postfix ++indexResult.

> Source/WebCore/svg/animation/SVGSMILElement.cpp:659
> +	   if (indexOfResult + 1 < sizeOfList - 1) {
> +	       return list[indexOfResult + 1];
>	   }

Braces are not needed.


More information about the webkit-reviews mailing list