Bug 136212 – [TexMap] Use std::sort instead of qsort in TextureMapperLayer::sortByZOrder()

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[TexMap] Use std::sort instead of qsort in TextureMapperLayer::sortByZOrder()

[Zan Dobersek]

Created attachment 237069 [details]
Patch

[WebKit Commit Bot]

Attachment 237069 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergio Villar Senin]

Comment on attachment 237069 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:157
> +            return a->m_centerZ < b->m_centerZ * 1000;

why "* 1000" ?

[Brent Fulgham]

It's not clear from this bug why the change is being proposed. Do you find that std::sort is quicker than qsort, or has better memory characteristics?

Could you include that in the ChangeLog to give context?

[Brent Fulgham]

Comment on attachment 237069 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:157
>> +            return a->m_centerZ < b->m_centerZ * 1000;
> 
> why "* 1000" ?

That's the same as was used in the original compareGraphicsLayerZValue method.

[Zan Dobersek]

(In reply to comment #4)
> It's not clear from this bug why the change is being proposed. Do you find
> that std::sort is quicker than qsort, or has better memory characteristics?
> 
> Could you include that in the ChangeLog to give context?

std::sort() can be faster due to ideally inlining the comparison function.

There are a couple of issues with rendering when std::sort() is used instead of qsort. I'll look into that first and then update the patch.

[Zan Dobersek]

Comment on attachment 237069 [details]
Patch

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

>>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:157
>>> +            return a->m_centerZ < b->m_centerZ * 1000;
>> 
>> why "* 1000" ?
> 
> That's the same as was used in the original compareGraphicsLayerZValue method.

I kept the same comparison that was used for qsort. But I don't know off the top of my head why 1000 is used here.

[Darin Adler]

Comment on attachment 237069 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:157
>>>> +            return a->m_centerZ < b->m_centerZ * 1000;
>>> 
>>> why "* 1000" ?
>> 
>> That's the same as was used in the original compareGraphicsLayerZValue method.
> 
> I kept the same comparison that was used for qsort. But I don't know off the top of my head why 1000 is used here.

That * 1000 is absolutely *not* the same as what was in the original function!

The original subtracts two floats and then multiplies the difference times 1000 to convert them to an integer that the sort algorithm can use. Look carefully at where the parentheses are.

This new code multiplies only b’s value by 1000. That’s not right.

[Zan Dobersek]

Comment on attachment 237069 [details]
Patch

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

>>>>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:157
>>>>> +            return a->m_centerZ < b->m_centerZ * 1000;
>>>> 
>>>> why "* 1000" ?
>>> 
>>> That's the same as was used in the original compareGraphicsLayerZValue method.
>> 
>> I kept the same comparison that was used for qsort. But I don't know off the top of my head why 1000 is used here.
> 
> That * 1000 is absolutely *not* the same as what was in the original function!
> 
> The original subtracts two floats and then multiplies the difference times 1000 to convert them to an integer that the sort algorithm can use. Look carefully at where the parentheses are.
> 
> This new code multiplies only b’s value by 1000. That’s not right.

Well, that turned out embarrassing.

Thanks for spotting it.

[Zan Dobersek]

Created attachment 244075 [details]
Patch

[WebKit Commit Bot]

Attachment 244075 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:156:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 244075 [details]
Patch

Clearing flags on attachment: 244075

Committed r178035: <http://trac.webkit.org/changeset/178035>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.