Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

CGbR project

I am currently working on a project that generates code from classes. The idea is to achieve maximum performance by replacing reflection use cases with fast, generated code.

Current results look good, but I want to take this chance to find room for further improvement.

Class under test:

[DataContract]
public partial class Partial
{
    [DataMember]
    public float Price { get; set; }

    [DataMember]
    public string Name { get; set; }

    [DataMember]
    public List<double> DecimalNumbers { get; set; }
}

Generated JSON

One of the generator creates methods to read and write JSON. Code is available in full at the benchmark project.

To JSON:

writer.Write(",\"Price\":");
writer.Write(Price.ToString(CultureInfo.InvariantCulture));

writer.Write(",\"Name\":");
writer.Write(string.Format("\"{0}\"", Name));

writer.Write(",\"DecimalNumbers\":");
if (DecimalNumbers == null)
    writer.Write("null");
else
{
    writer.Write('[');
    foreach (var value in DecimalNumbers)
    {
        writer.Write(value.ToString(CultureInfo.InvariantCulture));
        writer.Write(',');
    }
    writer.Write(']');
}

From JSON:

while (reader.Read())
{
    // Break on EndObject
    if (reader.TokenType == JsonToken.EndObject)
        break;

    // Only look for properties
    if (reader.TokenType != JsonToken.PropertyName)
        continue;

    switch ((string) reader.Value)
    {
        case "Price":
            reader.Read();
            Price = Convert.ToSingle(reader.Value);
            break;

        case "Name":
            reader.Read();
            Name = Convert.ToString(reader.Value);
            break;

        case "DecimalNumbers":
            reader.Read(); // Read token where array should begin
            if (reader.TokenType == JsonToken.Null)
                break;
            var decimalnumbers = new List<double>();
            while (reader.Read() && reader.TokenType != JsonToken.EndArray)
                decimalnumbers.Add(Convert.ToDouble(reader.Value));
            DecimalNumbers = decimalnumbers;
            break;

Generated binary conversion

Another generator maps the class directly onto a byte[].

Converter methods:

public static unsafe void Include(ushort value, byte[] bytes, int index)
{
    fixed (byte* b = bytes)
        *((ushort*)(b + index)) = value;
}

To binary:

// Convert Price
GeneratorByteConverter.Include(Price, bytes, index);;
index += 4;
// Convert Name
// Two bytes length information for each dimension
GeneratorByteConverter.Include((ushort)(Name == null ? 0 : Name.Length), bytes, index);
index += 2;
if (Name != null) Buffer.BlockCopy(_encoder.GetBytes(Name), 0, bytes, index, Name.Length);
index += Name == null ? 0 : Name.Length;
// Convert DecimalNumbers
// Two bytes length information for each dimension
GeneratorByteConverter.Include((ushort)(DecimalNumbers == null ? 0 : DecimalNumbers.Count), bytes, index);
index += 2;
if (DecimalNumbers != null)
for(var i = 0; i < DecimalNumbers.Count; i++)
{
    var value = DecimalNumbers[i];
    GeneratorByteConverter.Include(value, bytes, index);;
    index += 8;
}

From binary:

// Read Price
Price = BitConverter.ToSingle(bytes, index);
index += 4;
// Read Name
var nameLength = BitConverter.ToUInt16(bytes, index);
index += 2;
Name = _encoder.GetString(bytes, index, nameLength);
index += nameLength;
// Read DecimalNumbers
var decimalnumbersLength = BitConverter.ToUInt16(bytes, index);
index += 2;
var tempDecimalNumbers = new List<double>(decimalnumbersLength);
for (var i = 0; i < decimalnumbersLength; i++)
{
    var value = BitConverter.ToDouble(bytes, index);
    index += 8;
    tempDecimalNumbers.Add(value);
}
DecimalNumbers = tempDecimalNumbers;

I couldn't post all the code here, but maybe one of you already sees something I am missing.

share|improve this question
    
I'm not sure what code you want reviewed. – 200_success 2 days ago
    
The To/From methods if somebody spots potential for speed improvement. Last time I asked on SO and someone pointed out to use for instead of foreach. Back then I didn't know there is a dedicated community for questions like that. – Toxantron 2 days ago
    
Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. – Vogel612 2 days ago
    
I apologize. Coming from SO I thought the same mentality applies. – Toxantron 2 days ago
up vote 2 down vote accepted

Avoid single statement control structures

if (DecimalNumbers == null)
    writer.Write("null");
else

The one real argument in favor of the single statement version of control statements is that they save typing. However, in this case, it increases the amount of typing, as you have to add special logic to handle the single statement case.

if (DecimalNumbers == null)
{
    writer.Write("null");
}
else

If anyone ever edits the generated code directly, the single statement form can be actively harmful. Someone not paying attention can add a second statement and not realize that it is not part of a block. So a statement that is only supposed to run some of the time runs all the time.

Indentation?

if (DecimalNumbers != null)
for(var i = 0; i < DecimalNumbers.Count; i++)
{
    var value = DecimalNumbers[i];
    GeneratorByteConverter.Include(value, bytes, index);;
    index += 8;
}

Shouldn't most of these lines (all but the first) have another level of indent?

Note that this is an example of when the single statement form can be confusing. It would be more obvious that the for loop was inside a then block if it were in block form.

foreach vs. for

In .NET, which loop runs faster, 'for' or 'foreach'?

Most answers agree that for is faster than the equivalent foreach but not all. See here. Note that that answer is newer than others.

There is no reason why a foreach has to be slower than the equivalent for. The compiler can certainly make that conversion for you. Even if it is true for some case now, it may not be true in the future.

Another answer also points out that you would gain as much performance by switching from List to an array.

If performance is that important to you, then you should benchmark the various alternatives. You might even find it useful to have the code generator run benchmarks for you. Because apparently some situations are optimized and others not.

share|improve this answer
    
Thank you for taking the time to review it. I have to admit I was a little lazy about formatting the generated code, but I think you are right. And I agree that arrays are faster and the generator has special support for array. But in order to fit more needs all collections are supported. I will however point out in the documentation that arrays are faster. – Toxantron 2 days ago
    
To make List as fast as possible I eliminated internal resizing with new List<double>(decimalnumbersLength); – Toxantron 2 days ago

Unsafe unsafe

public static unsafe void Include(ushort value, byte[] bytes, int index)
{
    fixed (byte* b = bytes)
        *((ushort*)(b + index)) = value;
}

You're not making sure index + sizeof(ushort) is within the bytes array.

If these values will be sent between machines you should explicitly use either big or little endian and not rely on machine architectures being the same.

share|improve this answer
    
Skipping the check was done on purpose and the required size of bytes[] is calculated in advance, so this can't happen. However you have a point about big and little endian. – Toxantron yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.