patterncsharpMinor
RC4 implementation
Viewed 0 times
implementationrc4stackoverflow
Problem
I am using the following implementation of the RC4 encryption algorithm in C#
Is it possible to optimize my implementation a bit further, eventually using unsafe code, - especially for the variable swapping?
I hope for some constructive criticism.
///
/// RC4 encryption algorithm
///
/// Input bytes
/// Key bytes
/// encrypted bytes
public static byte[] Encrypt(byte[] bytes, byte[] key)
{
byte[] z = new byte[bytes.Length];
byte[] s = new byte[256];
byte[] k = new byte[256];
byte temp;
int i, j;
for (i = 0; i < 256; i++)
{
s[i] = (byte)i;
k[i] = key[i % key.GetLength(0)];
}
j = 0;
for (i = 0; i < 256; i++)
{
j = (j + s[i] + k[i]) % 256;
temp = s[i];
s[i] = s[j];
s[j] = temp;
}
i = j = 0;
for (int x = 0; x < z.GetLength(0); x++)
{
i = (i + 1) % 256;
j = (j + s[i]) % 256;
temp = s[i];
s[i] = s[j];
s[j] = temp;
int t = (s[i] + s[j]) % 256;
z[x] = (byte)(bytes[x] ^ s[t]);
}
return z;
}Is it possible to optimize my implementation a bit further, eventually using unsafe code, - especially for the variable swapping?
I hope for some constructive criticism.
Solution
public static byte[] Encrypt(byte[] bytes, byte[] key)bytes doesn't tell me anything that I don't already know from the type. plaintext would tell me the meaning of this variable.byte[] z = new byte[bytes.Length];Similarly, this would be more usefully named
ciphertext.byte[] s = new byte[256];
byte[] k = new byte[256];
byte temp;
int i, j;Is there any benefit to reusing
temp, i, and j rather than just redeclaring them in each scope? The benefit to redeclaring them is that it's more transparent that they are new variables.for (i = 0; i < 256; i++)
{
...
k[i] = key[i % key.GetLength(0)];
}
...
for (i = 0; i < 256; i++)
{
j = (j + s[i] + k[i]) % 256;
...
}k is used precisely once, and that's in a loop of the same length as the one which initialises it. IMO it would simplify things to eliminate the middleman and just use key[i % key.Length] in the second loop.j = (j + s[i] + k[i]) % 256;i = (i + 1) % 256;
j = (j + s[i]) % 256;
...
int t = (s[i] + s[j]) % 256;Here is the big optimisation opportunity. Replacing
% 256 with & 255 gives me a 25% reduction in execution time with a large plaintext.for (int x = 0; x < z.GetLength(0); x++)I missed this at first: since it's a one-dimensional array, you can use
Length instead of GetLength(0). This gives a significant further speedup.I must say, though, that the code is simple enough to already be very fast. My test case is 512MB of plaintext, and the time drops from 10.2 secs to 5.4 secs.
Code Snippets
public static byte[] Encrypt(byte[] bytes, byte[] key)byte[] z = new byte[bytes.Length];byte[] s = new byte[256];
byte[] k = new byte[256];
byte temp;
int i, j;for (i = 0; i < 256; i++)
{
...
k[i] = key[i % key.GetLength(0)];
}
...
for (i = 0; i < 256; i++)
{
j = (j + s[i] + k[i]) % 256;
...
}j = (j + s[i] + k[i]) % 256;Context
StackExchange Code Review Q#157603, answer score: 2
Revisions (0)
No revisions yet.