5
\$\begingroup\$

Is there any way that I can improve the performance of my x12_reader? The main functions that seem to be the bottlenecks are read_line() and get_element() they are both called upwards of millions of times.

using System;
using System.Collections.Generic;
using System.IO;
using System.Text;
using System.Windows.Forms;

namespace X12ReprocessApp {

public class segment {
    public String line_text = "";
    public String loop_id = "";
    public String id = null;

    ///<summary>
    ///Returns a string containing the element from the current segment.
    ///</summary>
    ///<param name="element_number">The element number that you want to return</param>
    public String get_element(int element_number) {
        int count = 0;
        int start_index = 0;
        int end_index = 0;
        int current_index = 0;

        while (count < element_number && current_index != -1) {
            current_index = line_text.IndexOf(x12_reader.element_delimiter, start_index);
            start_index = current_index + 1;
            count++;
        }

        if (current_index != -1) {
            end_index = line_text.IndexOf(x12_reader.element_delimiter, start_index);
            if (end_index == -1) end_index = line_text.Length;
            return line_text.Substring(start_index, end_index - start_index);
        } else {
            return "";
        }
    }

    ///<summary>
    ///Returns a decimal containing the element from the current segment.
    ///</summary>
    ///<param name="element_number">The element number that you want to return</param>
    public Decimal get_element_as_number(int element_number) {
        Decimal i = (Decimal.TryParse(get_element(element_number), out i)) ? i : 0;
        return i;
    }

    ///<summary>
    ///Return added total of the elements within the segment.
    ///</summary>
    ///<param name="start">Element number to start adding.</param>
    ///<param name="step">Increment of element position after each add.</param>
    public Decimal get_elements_total(int start, int step) {
        Decimal total = 0;
        for (int i = start; i <= get_length(); i += step) {
            total += get_element_as_number(i);
        }
        return total;
    }

    public String get_id() {
        if (id != null) return id;
        int index = line_text.IndexOf(x12_reader.element_delimiter);
        if (index >= 0) {
            id = line_text.Substring(0, index);
            return id;
        }
        return String.Empty;
    }

    public int get_length() {
        int count = 1;
        for (int i = 0; i < line_text.Length; i++) {
            if (line_text[i] == x12_reader.element_delimiter) count++;
        }
        return count;
    }

    ///<summary>
    ///Returns a segment with the selected element replaced with the string value passed.
    ///</summary>
    ///<param name="element_number">The element that you want to replace.</param>
    ///<param name="value">The string value with which you want to replace the element.</param>
    public segment replace_element(int element_number, String value) {
        String[] elements = line_text.Split(x12_reader.element_delimiter);
        if (element_number < elements.Length && element_number > 0) {
            elements[element_number] = value;
            segment return_segment = new segment();
            return_segment.line_text = String.Join(x12_reader.element_delimiter.ToString(), elements);
            return return_segment;
        }
        return this;
    }
}

public class transaction {
    public segment[] segments;

    ///<summary>
    //Returns true if the current transaction contains the id.
    ///</summary>
    ///<param name="id">The id to find.</param>
    ///<param name="in_loop">The loop to limit the search.</param>
    public bool contains_id(String id, String in_loop = null) {
        foreach (segment s in segments) {
            if (in_loop != null) {
                if (s.get_id() == id && s.loop_id == in_loop) return true;
            } else if (s.get_id() == id) return true;
        }
        return false;
    }

    ///<summary>
    ///Returns the full text of the transaction as a String.
    ///</summary>
    public String get_full_text() {
        return get_lines(0, segments.Length - 1);
    }

    ///<summary>
    ///Returns a String containing a section of a transaction.
    ///</summary>
    public String get_lines(int start, int end) {
        StringBuilder sb = new StringBuilder();
        if (end >= segments.Length) end = segments.Length - 1;
        if (start >= 0) {
            for (int i = start; i <= end; i++) {
                sb.Append(segments[i].line_text);
                sb.AppendLine(x12_reader.line_terminator.ToString());
            }
            return sb.ToString();
        }
        return "";
    }

    ///<summary>
    ///Returns the first segment of the id type requested.
    ///</summary>
    ///<param name="id">ID of the segment to return.</param>
    ///<param name="element_1">First element of the ID to return.</param>
    public segment get_segment_of_type(String id, String element_1 = null) {
        foreach (segment s in segments) {
            if (s.get_id() == id) {
                if (element_1 != null) {
                    if (element_1 == s.get_element(1)) {
                        return s;
                    }
                } else {
                    return s;
                }
            }
        }
        return null;
    }

    ///<summary>
    ///Returns a List containing only segments with the corresponding id.
    ///</summary>
    public List<segment> get_segments_of_type(String id, String element_1 = null) {
        List<segment> temp_segment_list = new List<segment>();
        foreach (segment s in segments) {
            if (s.get_id() == id) {
                if (element_1 != null) {
                    if (element_1 == s.get_element(1)) {
                        temp_segment_list.Add(s);
                    }
                } else {
                    temp_segment_list.Add(s);
                }
            }
        }
        return temp_segment_list;
    }
}

public class x12_reader {
    public static Char element_delimiter;
    public static Char line_terminator;
    public static Char sub_delimiter;
    public segment GE;
    public segment GS;
    public Dictionary<String, Int32> id_counts = new Dictionary<String, Int32>();
    public segment IEA;
    public segment ISA;
    private FileStream file_stream;
    private StreamReader stream_reader;
    private StringBuilder string_builder = new StringBuilder();

    ///<summary>
    ///Class created to read and manage X12/EDI files.
    ///<param name="input">Path to the x12 file to be processed.</param>
    public x12_reader(String input) {
        file_stream = File.Open(input, FileMode.Open, FileAccess.Read);
        stream_reader = new StreamReader(file_stream, System.Text.Encoding.UTF8, false, 4096);

        line_terminator = read_char_at_location(105 + byte_order_mark_offset());
        sub_delimiter = read_char_at_location(104 + byte_order_mark_offset());
        element_delimiter = read_char_at_location(103 + byte_order_mark_offset());
    }

    ///<summary>
    ///Returns the number for segments with the selected id.
    ///</summary>
    ///<param name="id">The segment id to count.</param>
    public int get_segment_count(String id) {
        if (id_counts.ContainsKey(id)) return id_counts[id];
        return 0;
    }

    ///<summary>
    ///Builds the transactions list within the x12_reader class.
    ///</summary>
    public IEnumerable<transaction> read_x12(ProgressBar pBar = null) {
        MethodInvoker m = new MethodInvoker(() => pBar.Maximum = (int)stream_reader.BaseStream.Length);
        MethodInvoker v = new MethodInvoker(() => pBar.Value = (int)stream_reader.BaseStream.Position);

        int update_count = 0;
        List<segment> segments = new List<segment>();
        segment s;
        String current_loop = "";
        transaction t = new transaction();

        pBar.Invoke(m);
        using (stream_reader) {
            while (!stream_reader.EndOfStream) {
                update_count++;
                if (update_count >= 150000) {
                    pBar.Invoke(v);
                    update_count = 0;
                }

                s = get_segment();
                switch (s.get_id()) {
                    case "ISA":
                        ISA = s;
                        ISA.line_text = ISA.line_text.Substring(byte_order_mark_offset(), ISA.line_text.Length - byte_order_mark_offset());
                        ISA.loop_id = current_loop;
                        break;

                    case "IEA":
                        IEA = s;
                        IEA.loop_id = current_loop;
                        break;

                    case "GS":
                        GS = s;
                        GS.loop_id = current_loop;
                        break;

                    case "GE":
                        GE = s;
                        GE.loop_id = current_loop;
                        break;

                    case "ST":
                        current_loop = "";
                        s.loop_id = current_loop;
                        segments.Clear();
                        t = new transaction();
                        segments.Add(s);
                        break;

                    case "SE":
                        current_loop = "";
                        s.loop_id = current_loop;
                        segments.Add(s);
                        t.segments = segments.ToArray();
                        yield return t;
                        break;

                    case "N1":
                        if (s.get_element(1) == "PR") current_loop = "1000A";
                        if (s.get_element(1) == "PE") current_loop = "1000B";
                        s.loop_id = current_loop;
                        segments.Add(s);
                        break;

                    case "LX":
                        current_loop = "2000";
                        s.loop_id = current_loop;
                        segments.Add(s);
                        break;

                    case "CLP":
                        current_loop = "2100";
                        s.loop_id = current_loop;
                        segments.Add(s);
                        break;

                    case "SVC":
                        current_loop = "2110";
                        s.loop_id = current_loop;
                        segments.Add(s);
                        break;

                    case "PLB":
                        current_loop = "";
                        s.loop_id = current_loop;
                        segments.Add(s);
                        break;

                    default:
                        s.loop_id = current_loop;
                        segments.Add(s);
                        break;
                }
            }
            pBar.Invoke(v);
        }
    }

    private int byte_order_mark_offset() {
        if (read_char_at_location(0) == 0xEF &&
            read_char_at_location(1) == 0xBB &&
            read_char_at_location(2) == 0xBF) {
            return 3;
        }
        return 0;
    }

    private segment get_segment() {
        segment segment = new segment();
        segment.line_text = read_line();
        increment_count_for_id(segment.get_id());
        return segment;
    }

    private void increment_count_for_id(String id) {
        int value = 0;
        if (!id_counts.TryGetValue(id, out value)) id_counts.Add(id, 0);
        id_counts[id]++;
    }

    private Char read_char_at_location(long location) {
        long old_location = file_stream.Position;
        file_stream.Position = location;
        if (file_stream.Position < file_stream.Length) {
            Char c = (char)file_stream.ReadByte();
            file_stream.Position = old_location;
            return c;
        }
        file_stream.Position = old_location;
        return '\0';
    }

    private String read_line() {
        string_builder.Clear();
        int n;
        while ((n = stream_reader.Read()) != -1) {
            if (n == line_terminator) return string_builder.ToString();
            string_builder.Append((char)n);
        }
        return string_builder.ToString();
    }
}
}
  1. I can not make get_element() return multiple elements at once. The function must return one element at at time.

  2. I have already run a profiler. My choke points seem to be Substring and StringBuilder.

  3. I have already tried to reduce the amount of times I call get_element() and read_line() functions in my calling code. I do not think I can reduce the amount of calls any further.

Example of use:

x12_reader xr = new x12_reader(path_to_x12);

foreach (transaction t in xr.read_x12(_progressbar_all_processing)) {
        //Do something with the transaction
        foreach(segment s in t.segments){
               if(s.get_id() == "SOMEID"){
                   s.get_element(5);
               }
        }
}

I've come to the conclusion that there really is no room left here for performance improvements. It currently reads around 700,000 segments a second. I will have to try to get performance improvements by threading some things, or installing improved hardware on the machine that will be running this. Thanks everyone for your help.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ Where did you learn this writing style? Your code is really hard to read. \$\endgroup\$
    – t3chb0t
    Aug 31, 2016 at 13:09
  • 1
    \$\begingroup\$ @t3chb0t, it's not harder to read than any other C program. So maybe it's just harder to read for you, because you are used to a single coding style (C#, I guess). OP's style might be different, but it's 100% consistent -- which is what actually matters. \$\endgroup\$ Sep 3, 2016 at 15:32
  • 1
    \$\begingroup\$ @Daniel I agree with GoodNight. Consistency is most important and your code is consistent. Real world though, many language communities do have style "standards" that are expected by other users of the language. C# programmers often default to Microsoft's C# Naming Guidelines and Coding Conventions, particularly in a professional setting. \$\endgroup\$
    – RubberDuck
    Sep 3, 2016 at 16:09
  • 1
    \$\begingroup\$ That said, one thing that is objectively hard to read is this line. segment segment = new segment(); It's one good reason to follow the naming standards. You used a variable with the same case and name as a type. I won't answer though because I'd like to see you get a performance related answer. \$\endgroup\$
    – RubberDuck
    Sep 3, 2016 at 16:16
  • 1
    \$\begingroup\$ Lastly, don't roll your own when there are prebuilt solutions that fit your need. I can't promise this project will meet your performance requirements, but it might. There's a NuGet package available. x12parser.codeplex.com \$\endgroup\$
    – RubberDuck
    Sep 3, 2016 at 16:21

2 Answers 2

3
\$\begingroup\$

Some quick remarks WRT your coding style, which doesn't follow Microsoft's guidelines:

  • segment: class names should be PascalCase.
  • get_element: method names should be PascalCase.
  • start_index: don't use non-alphanumeric characters in method names, class names, etc.
  • String, Decimal, Int32,...: usually the aliases are preferred (string, decimal, int,...).
  • get_element_as_number(int element_number): parameters should be camelCase.
  • public segment[] segments;: properties should be PascalCase and should have a getter and/or a setter.
\$\endgroup\$
5
  • \$\begingroup\$ -1 How is that gonna improve performance? OP specifically asked for help with improving the performance as it says in the title, in the question text, and in the tags. So why would you consider this a valid answer to OP's question in the first place? \$\endgroup\$ Sep 3, 2016 at 15:28
  • \$\begingroup\$ @GoodNightNerdPride good job sheriff :-| \$\endgroup\$
    – t3chb0t
    Sep 3, 2016 at 15:45
  • \$\begingroup\$ @GoodNight posting here you get a complete review. If you have a specific question you may post on Stack Overflow. This answer may not be complete but it's right and often what OP is asking is not really relevant, other answers may address other issues and someone may also talk about performance. All together you have a code review, just like in real life CR sessions... \$\endgroup\$ Sep 3, 2016 at 16:09
  • \$\begingroup\$ "If you have a specific question you may post on Stack Overflow." (@AdrianoRepetti) That's funny, because on SO they said he should ask on CR. So what now? Also isn't the performance tag saying "This question is about performance, hence I'm not interested in coding style recommendations"? So why waste everybody's time with this stuff then? \$\endgroup\$ Sep 3, 2016 at 17:59
  • \$\begingroup\$ Usually performance questions are pretty on topic for Stack Overflow, they are more borderline here (according to FAQ) if to fix your poor performance code is the only thing you want. \$\endgroup\$ Sep 3, 2016 at 19:16
2
\$\begingroup\$

Coding style aside - your code is very hard to follow, you didn't show us any sample data and you didn't show us how to run it - your code is quite long and without being able to run it don't expect wonders - here are the reasons why:

x12_reader class

You use global fields like file_stream, stream_reader or string_builder (and the others GE, GS....) that you initialize somewhere and modify in other places. They don't have to be global at all. You could use them only where you really need them. StringBuilder should be a local variable in the read_line method.

performance

segment class

You probably cannot reduce calls to get_element but what you can do is to when you create this class you should split all fields by the delimiter and put them into an array so you don't have to search through the line every time you read a data-field. Just get it from the array by index. This way you won't have to call Substring each time you get a field.

If you do this then you can reduce the get_element to simple:

return elements[element_number];

Other that this I cannot help unless you provide more information. Perhaps there is a better way to parse your data but without samples you'll probably won't get anymore advises.

\$\endgroup\$
6
  • \$\begingroup\$ Reusing StringBuilder is ugly design but good for performance (especially if size grows few times). OK pooling is even better... \$\endgroup\$ Sep 3, 2016 at 18:38
  • \$\begingroup\$ @AdrianoRepetti this might be true but I don't believe that that's the reason here - if it was for performance then it would have been initialized with a capacity but it isn't. \$\endgroup\$
    – t3chb0t
    Sep 3, 2016 at 18:41
  • \$\begingroup\$ I agree, I'm talking in general. I don't think OP thought about it (but I may be wrong, he already profiled his code and found StringBuilder is CPU consuming) \$\endgroup\$ Sep 3, 2016 at 19:13
  • \$\begingroup\$ @t3chb0t, maybe the default capacity (16) of the StringBuilder is just perfectly fine for OP's needs? Looking at some X12 it appears to be for me at least. \$\endgroup\$ Sep 4, 2016 at 14:04
  • \$\begingroup\$ @t3chb0t, regarding the use of string.Split(): as far as I understand it OP wants to retrieve one or two elements of something like a million segments each. Doing that split for each segment instead of just searching for the specified element(s) in the segment string seems to be a waste of both CPU cycles and memory as most of the elements won't be used anyway. \$\endgroup\$ Sep 4, 2016 at 14:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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