search

redis / jedis

Redis Java client
MIT License
11.79k stars 3.86k forks source link

Consider using faster CRC16 algorithm #729

Closed allanwax closed 10 years ago

allanwax commented 10 years ago

SEE http://introcs.cs.princeton.edu/java/51data/CRC16.java.html

HeartSaVioR commented 10 years ago

@allanwax Oh, it should be faster then current!

Btw, it seems to not equal to cluster-spec's Appendix A: CRC16 reference implementation in ANSI C. Would you try a benchmark to compare current, your link, ported 'Georges Menie's implementation'? Thanks for your effort!

HeartSaVioR commented 10 years ago

@allanwax Btw, how about learning essential git commands and contributing by code? I wish to leave your effort for JedisCluster to Jedis Project. ;)

mp911de commented 10 years ago

The mentioned CRC16 impl does not match Redis' CRC 16. Redis uses XMODEM with a Polynominal of 1021 (x16 + x12 + x5 + 1). Test value within the Redis Cluster spec says CRC16("123456789") = 0x31C3. The linked results in CRC16("123456789") = 0xBB3D

allanwax commented 10 years ago

Thanks. I was trying to find a way to speed up the CRC from the current code since getting the slot number is heavily used. I'll look elsewhere for something that is compatible.

On 9/11/2014 8:40 AM, Mark Paluch wrote:

The mentioned CRC16 impl does not match Redis' CRC 16. Redis uses XMODEM with a Polynominal of |1021 (x16 + x12 + x5 + 1)|. Test value within the Redis Cluster spec says |CRC16("123456789") = 0x31C3|. The linked results in |CRC16("123456789") = 0xBB3D|

— Reply to this email directly or view it on GitHub https://github.com/xetorthio/jedis/issues/729#issuecomment-55282809.

HeartSaVioR commented 10 years ago

@mp911de I can find your implementation of CRC using lookup table from your forked version of lettuce. If you don't mind, can you craft PR with your version of CRC? Or can I borrow your implementation?

Other way we can try to port Redis Cluster specification's Appendix A to Java. @allanwax What do you think? :)

mp911de commented 10 years ago

@HeartSaVioR Feel free to use it. There are also some tests.

HeartSaVioR commented 10 years ago

@mp911de Thanks! I'll apply your implementation and post PR.

HeartSaVioR commented 10 years ago

I've made a small benchmark, and compared each other. (current vs @mp911de 's)

public class CRC16Benchmark {
    private static final int TOTAL_OPERATIONS = 10000000;

    private static String[] TEST_SET = {
    "", "123456789", "sfger132515", "hae9Napahngaikeethievubaibogiech", 
    "AAAAAAAAAAAAAAAAAAAAAA", "Hello, World!"
    };

    public static void main(String[] args) throws UnknownHostException,
    IOException {
    long begin = Calendar.getInstance().getTimeInMillis();

    for (int n = 0; n <= TOTAL_OPERATIONS; n++) {
        JedisClusterCRC16.getSlot(TEST_SET[n % TEST_SET.length]);
    }

    long elapsed = Calendar.getInstance().getTimeInMillis() - begin;
    System.out.println(((1000 * TOTAL_OPERATIONS) / elapsed) + " ops");
    }
}

Before applying (current)

449208 ops 434400 ops 448351 ops 428461 ops 430422 ops

After applying @mp911de 's implementation

1053073 ops 1048375 ops 1061796 ops 1057020 ops 1053073 ops

Test machine is Macbook Pro - 2.3G i7, DDR3 16G, OSX 10.9.4. I ran benchmark with STS.

mp911de commented 10 years ago

more is better?

HeartSaVioR commented 10 years ago

@mp911de Yes, your implementation is more than 2x faster from current implementation!

allanwax commented 10 years ago

I think the port of the algorithm in the spec is a good way to go but testing data is needed.

Also, when doing so, remember that a table lookup is always faster than any calculation you can do.

As to the algorithm I posted (http://introcs.cs.princeton.edu/java/51data/CRC16.java.html), it was not written by me and Jungtaek correctly points out that the one I referenced uses a different irreducible polynomial.

Allan Wax

On 9/11/2014 3:31 PM, Jungtaek Lim wrote:

@mp911de https://github.com/mp911de I can find your implementation of CRC using lookup table from your forked version of lettuce. If you don't mind, can you craft PR with your version of CRC? Or can I borrow your implementation?

Other way we can try to port Redis Cluster specification's Appendix A to Java. @allanwax https://github.com/allanwax What do you think? :)

— Reply to this email directly or view it on GitHub https://github.com/xetorthio/jedis/issues/729#issuecomment-55338511.

marcosnils commented 10 years ago

Hi Allan, the improvement @mp911de mentioned was introduced in #733.

I'm closing the issue.

allanwax commented 10 years ago

Here's my implementation of it. Needs more testing. (I'm not a git person).

public class CRC16 { private static final int crc16tab[] = {// // 0x0000, 0x1021, 0x2042, 0x3063, 0x4084, 0x50a5, 0x60c6, 0x70e7,// 0x8108, 0x9129, 0xa14a, 0xb16b, 0xc18c, 0xd1ad, 0xe1ce, 0xf1ef,// 0x1231, 0x0210, 0x3273, 0x2252, 0x52b5, 0x4294, 0x72f7, 0x62d6,// 0x9339, 0x8318, 0xb37b, 0xa35a, 0xd3bd, 0xc39c, 0xf3ff, 0xe3de,// 0x2462, 0x3443, 0x0420, 0x1401, 0x64e6, 0x74c7, 0x44a4, 0x5485,// 0xa56a, 0xb54b, 0x8528, 0x9509, 0xe5ee, 0xf5cf, 0xc5ac, 0xd58d,// 0x3653, 0x2672, 0x1611, 0x0630, 0x76d7, 0x66f6, 0x5695, 0x46b4,// 0xb75b, 0xa77a, 0x9719, 0x8738, 0xf7df, 0xe7fe, 0xd79d, 0xc7bc,// 0x48c4, 0x58e5, 0x6886, 0x78a7, 0x0840, 0x1861, 0x2802, 0x3823,// 0xc9cc, 0xd9ed, 0xe98e, 0xf9af, 0x8948, 0x9969, 0xa90a, 0xb92b,// 0x5af5, 0x4ad4, 0x7ab7, 0x6a96, 0x1a71, 0x0a50, 0x3a33, 0x2a12,// 0xdbfd, 0xcbdc, 0xfbbf, 0xeb9e, 0x9b79, 0x8b58, 0xbb3b, 0xab1a,// 0x6ca6, 0x7c87, 0x4ce4, 0x5cc5, 0x2c22, 0x3c03, 0x0c60, 0x1c41,// 0xedae, 0xfd8f, 0xcdec, 0xddcd, 0xad2a, 0xbd0b, 0x8d68, 0x9d49,// 0x7e97, 0x6eb6, 0x5ed5, 0x4ef4, 0x3e13, 0x2e32, 0x1e51, 0x0e70,// 0xff9f, 0xefbe, 0xdfdd, 0xcffc, 0xbf1b, 0xaf3a, 0x9f59, 0x8f78,// 0x9188, 0x81a9, 0xb1ca, 0xa1eb, 0xd10c, 0xc12d, 0xf14e, 0xe16f,// 0x1080, 0x00a1, 0x30c2, 0x20e3, 0x5004, 0x4025, 0x7046, 0x6067,// 0x83b9, 0x9398, 0xa3fb, 0xb3da, 0xc33d, 0xd31c, 0xe37f, 0xf35e,// 0x02b1, 0x1290, 0x22f3, 0x32d2, 0x4235, 0x5214, 0x6277, 0x7256,// 0xb5ea, 0xa5cb, 0x95a8, 0x8589, 0xf56e, 0xe54f, 0xd52c, 0xc50d,// 0x34e2, 0x24c3, 0x14a0, 0x0481, 0x7466, 0x6447, 0x5424, 0x4405,// 0xa7db, 0xb7fa, 0x8799, 0x97b8, 0xe75f, 0xf77e, 0xc71d, 0xd73c,// 0x26d3, 0x36f2, 0x0691, 0x16b0, 0x6657, 0x7676, 0x4615, 0x5634,// 0xd94c, 0xc96d, 0xf90e, 0xe92f, 0x99c8, 0x89e9, 0xb98a, 0xa9ab,// 0x5844, 0x4865, 0x7806, 0x6827, 0x18c0, 0x08e1, 0x3882, 0x28a3,// 0xcb7d, 0xdb5c, 0xeb3f, 0xfb1e, 0x8bf9, 0x9bd8, 0xabbb, 0xbb9a,// 0x4a75, 0x5a54, 0x6a37, 0x7a16, 0x0af1, 0x1ad0, 0x2ab3, 0x3a92,// 0xfd2e, 0xed0f, 0xdd6c, 0xcd4d, 0xbdaa, 0xad8b, 0x9de8, 0x8dc9,// 0x7c26, 0x6c07, 0x5c64, 0x4c45, 0x3ca2, 0x2c83, 0x1ce0, 0x0cc1,// 0xef1f, 0xff3e, 0xcf5d, 0xdf7c, 0xaf9b, 0xbfba, 0x8fd9, 0x9ff8,// 0x6e17, 0x7e36, 0x4e55, 0x5e74, 0x2e93, 0x3eb2, 0x0ed1, 0x1ef0 // };

public static int crc16(final byte[] bytes) {
    int crc = 0;
    for (int i = 0; i < bytes.length; i++)
        crc = (crc << 8) ^ crc16tab[((crc >>> 8) ^ bytes[i]) & 0x00FF];
    return crc & 0xFFFF;
}

public static void main(String[] args) {

System.out.println(Integer.toHexString(crc16("123456789".getBytes()))); } }

On 9/11/2014 3:31 PM, Jungtaek Lim wrote:

@mp911de https://github.com/mp911de I can find your implementation of CRC using lookup table from your forked version of lettuce. If you don't mind, can you craft PR with your version of CRC? Or can I borrow your implementation?

Other way we can try to port Redis Cluster specification's Appendix A to Java. @allanwax https://github.com/allanwax What do you think? :)

— Reply to this email directly or view it on GitHub https://github.com/xetorthio/jedis/issues/729#issuecomment-55338511.

allanwax commented 10 years ago

(y)