93370 2012-08-07 07:36:01 -0700 reimplement fastMod w/o (soon to be) private skia macros 2012-08-07 15:09:25 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 reed reed enne jamesr schenney senorblanco webkit.review.bot oldest_to_newest 689131 0 reed 2012-08-07 07:36:01 -0700 reimplement fastMod w/o (soon to be) private skia macros 689133 1 156939 reed 2012-08-07 07:37:55 -0700 Created attachment 156939 Patch 689218 2 156939 enne 2012-08-07 11:25:07 -0700 Comment on attachment 156939 Patch R=me. Those look functionally equivalent to me. 689240 3 156939 webkit.review.bot 2012-08-07 11:45:03 -0700 Comment on attachment 156939 Patch Clearing flags on attachment: 156939 Committed r124901: <http://trac.webkit.org/changeset/124901> 689241 4 webkit.review.bot 2012-08-07 11:45:07 -0700 All reviewed patches have been landed. Closing bug. 689346 5 156939 darin 2012-08-07 13:51:21 -0700 Comment on attachment 156939 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156939&action=review > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:76 > + value = -value; This won’t work if value is MIN_INT. I assume that’s OK since the old code already worked like that. 689446 6 156939 allan.jensen 2012-08-07 14:58:50 -0700 Comment on attachment 156939 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156939&action=review > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:72 > inline int fastMod(int value, int max) Considering this a slow workaround for a something poorly defined in the C++ standard, I find the name fastMod a bit confusing. Have you considered a rename? I know this function is not from your patch, but since your patch makes it even slower it just makes the name weirder. Also do you actually have any examples of actual C++ implementations where (a % x) != -(-a % x). I thought this issue would be resolved by now. C++11 defined it to what all sane implementation already did, which is the same this function tries to return. 689459 7 reed 2012-08-07 15:09:25 -0700 I think fastMod came from the >= check, so that we don't call % at all if value < max Good question if there are any real differences in how negatives are handled w.r.t. %. I haven't looked in a long while. Not sure the new one is slower actually. On architectures that have conditional instructions like ARM (and modern x86s), if (x < 0) x = -x; Can be two instructions w/ zero branches, as opposed to 2 or 3 for the extract/apply pattern. I haven't timed this particular use case tho. However, I agree that fastMod does not actually capture the whole intent: safeIntegerMod might be a better name. 156939 2012-08-07 07:37:55 -0700 2012-08-07 14:58:50 -0700 Patch bug-93370-20120807103730.patch text/plain 1703 reed SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyNDg4NykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDEyLTA4LTA3ICBNaWtlIFJl ZWQgIDxyZWVkQGdvb2dsZS5jb20+CisKKyAgICAgICAgcmVpbXBsZW1lbnQgZmFzdE1vZCB3L28g KHNvb24gdG8gYmUpIHByaXZhdGUgc2tpYSBtYWNyb3MKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTkzMzcwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgZmFzdE1vZCgpIHJlaW1wbGVtZW50ZWQgKHNhbWUgZnVu Y3Rpb25hbGl0eSkgdG8gc3RvcCB1c2luZyBzb29uLXRvLWJlIHByaXZhdGUgbWFjcm9zIGluCisg ICAgICAgIFNrTWF0aC5oLiBUaGUgbmV3IHZlcnNpb24gaXMgZnVuY3Rpb25hbGx5IGlkZW50aWNh bC4KKworICAgICAgICBObyBuZXcgdGVzdHMgLS0gZXhpc3RpbmcgbGF5b3V0dGVzdHMgZXhlcmNp c2UgR3JhcGhpY3NDb250ZXh0OjpzdHJva2VBcmMoKSwgdGhlIG9ubHkgY2FsbGVyCisKKyAgICAg ICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9za2lhL0dyYXBoaWNzQ29udGV4dFNraWEuY3BwOgorCiAy MDEyLTA4LTA3ICBWc2V2b2xvZCBWbGFzb3YgIDx2c2V2aWtAY2hyb21pdW0ub3JnPgogCiAgICAg ICAgIFdlYiBJbnNwZWN0b3I6IERvIG5vdCBkaXNhYmxlIG5ldHdvcmsgdHJhY2tpbmcgd2hpbGUg cHJvZmlsaW5nIGNwdS4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3Nr aWEvR3JhcGhpY3NDb250ZXh0U2tpYS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9HcmFwaGljc0NvbnRleHRTa2lhLmNwcAkocmV2aXNpb24g MTI0ODg2KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9HcmFwaGlj c0NvbnRleHRTa2lhLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNjgsMTQgKzY4LDE5IEBAIG5hbWVz cGFjZSBXZWJDb3JlIHsKIAogbmFtZXNwYWNlIHsKIAorLy8gUmV0dXJuIHZhbHVlICUgbWF4LCBi dXQgYWNjb3VudCBmb3IgdmFsdWUgcG9zc2libHkgYmVpbmcgbmVnYXRpdmUuCiBpbmxpbmUgaW50 IGZhc3RNb2QoaW50IHZhbHVlLCBpbnQgbWF4KQogewotICAgIGludCBzaWduID0gU2tFeHRyYWN0 U2lnbih2YWx1ZSk7Ci0KLSAgICB2YWx1ZSA9IFNrQXBwbHlTaWduKHZhbHVlLCBzaWduKTsKKyAg ICBib29sIGlzTmVnID0gZmFsc2U7CisgICAgaWYgKHZhbHVlIDwgMCkgeworICAgICAgICB2YWx1 ZSA9IC12YWx1ZTsKKyAgICAgICAgaXNOZWcgPSB0cnVlOworICAgIH0KICAgICBpZiAodmFsdWUg Pj0gbWF4KQogICAgICAgICB2YWx1ZSAlPSBtYXg7Ci0gICAgcmV0dXJuIFNrQXBwbHlTaWduKHZh bHVlLCBzaWduKTsKKyAgICBpZiAoaXNOZWcpCisgICAgICAgIHZhbHVlID0gLXZhbHVlOworICAg IHJldHVybiB2YWx1ZTsKIH0KIAogaW5saW5lIGZsb2F0IHNxdWFyZShmbG9hdCBuKQo=