[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