2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'm not sure what code you want reviewed. \$\endgroup\$ – 200_success Apr 14 '16 at 23:19
  • \$\begingroup\$ 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. \$\endgroup\$ – Toxantron Apr 14 '16 at 23:25
  • \$\begingroup\$ 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. \$\endgroup\$ – Vogel612 Apr 15 '16 at 7:02
  • \$\begingroup\$ I apologize. Coming from SO I thought the same mentality applies. \$\endgroup\$ – Toxantron Apr 15 '16 at 7:03
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Toxantron Apr 15 '16 at 6:53
  • \$\begingroup\$ To make List as fast as possible I eliminated internal resizing with new List<double>(decimalnumbersLength); \$\endgroup\$ – Toxantron Apr 15 '16 at 7:00
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Toxantron Apr 15 '16 at 11:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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