RESOLVED FIXED 93370
reimplement fastMod w/o (soon to be) private skia macros
https://bugs.webkit.org/show_bug.cgi?id=93370
Summary reimplement fastMod w/o (soon to be) private skia macros
Mike Reed
Reported 2012-08-07 07:36:01 PDT
reimplement fastMod w/o (soon to be) private skia macros
Attachments
Patch (1.66 KB, patch)
2012-08-07 07:37 PDT, Mike Reed
no flags
Mike Reed
Comment 1 2012-08-07 07:37:55 PDT
Adrienne Walker
Comment 2 2012-08-07 11:25:07 PDT
Comment on attachment 156939 [details] Patch R=me. Those look functionally equivalent to me.
WebKit Review Bot
Comment 3 2012-08-07 11:45:03 PDT
Comment on attachment 156939 [details] Patch Clearing flags on attachment: 156939 Committed r124901: <http://trac.webkit.org/changeset/124901>
WebKit Review Bot
Comment 4 2012-08-07 11:45:07 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 5 2012-08-07 13:51:21 PDT
Comment on attachment 156939 [details] 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.
Allan Sandfeld Jensen
Comment 6 2012-08-07 14:58:50 PDT
Comment on attachment 156939 [details] 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.
Mike Reed
Comment 7 2012-08-07 15:09:25 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.