Bug 93370 - reimplement fastMod w/o (soon to be) private skia macros
Summary: reimplement fastMod w/o (soon to be) private skia macros
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike Reed
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-07 07:36 PDT by Mike Reed
Modified: 2012-08-07 15:09 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.66 KB, patch)
2012-08-07 07:37 PDT, Mike Reed
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Reed 2012-08-07 07:36:01 PDT
reimplement fastMod w/o (soon to be) private skia macros
Comment 1 Mike Reed 2012-08-07 07:37:55 PDT
Created attachment 156939 [details]
Patch
Comment 2 Adrienne Walker 2012-08-07 11:25:07 PDT
Comment on attachment 156939 [details]
Patch

R=me.  Those look functionally equivalent to me.
Comment 3 WebKit Review Bot 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>
Comment 4 WebKit Review Bot 2012-08-07 11:45:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Darin Adler 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.
Comment 6 Allan Sandfeld Jensen 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.
Comment 7 Mike Reed 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.