[TexMap] Use std::sort instead of qsort in TextureMapperLayer::sortByZOrder()
Created attachment 237069 [details] Patch
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.
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" ?
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?
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.
(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.
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.
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.
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.
Created attachment 244075 [details] Patch
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.
Comment on attachment 244075 [details] Patch Clearing flags on attachment: 244075 Committed r178035: <http://trac.webkit.org/changeset/178035>
All reviewed patches have been landed. Closing bug.