5
\$\begingroup\$

My application relies on an unknown number of DLLs that each produce a report. The only thing that is known in advance is the location of the files and the classes that each will contain, including the base types. When the user needs to define a new report for use in the application, a list of every available report is produced from which the user selects. This list is produced using the following code. I would like to know if you can point out any ways that I might increase the efficiency of this method. It currently processes each DLL (there are ~150) in 50ms or less (~30ms avg) on my machine (Core i7-4790 @ 3.6GHz, 8GB RAM, 64-bit Windows 10 Pro) for a total time of approximately 5 seconds. This isn't bad, but my development machine is pretty new and my users tend to not be so lucky. They could be running 5+ year old machines, and I fear the time-to-process could increase dramatically in the real world.

public void ProduceReportList()
{
    var path = Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "rpt_dll");
    var files = Directory.GetFiles(path, "*.dll", SearchOption.TopDirectoryOnly)
        .Where(x =>
            !Path.GetFileName(x).ToLower().StartsWith("rpt_") &&
            !Path.GetFileName(x).ToLower().StartsWith("rpt~") &&
            !Path.GetFileName(x).ToLower().StartsWith("_")).ToArray();
    foreach (var file in files)
    {
        if (NativeMethods.FileIsCLRAssembly(file))
        {
            Assembly theDLL = null;
            var bytes = File.ReadAllBytes(file);
            if (bytes.Length > 0)
                theDLL = Assembly.Load(bytes);
            if (theDLL != null)
            {
                foreach (var t in theDLL.GetTypes())
                {
                    if (typeof(baseReport).IsAssignableFrom(t))
                    {
                        using (var rptDLL = (baseReport)Activator.CreateInstance(t))
                        {
                            if (rptDLL != null)
                            {
                                //Report is validated and added to a list if it passes
                            }
                        }
                    }
                }
            }
        }
    }
}

//Helper method that attempts to determine whether the supplied FilePath points to a valid CLR assembly
public static Boolean FileIsCLRAssembly(String FilePath)
{
    Boolean retVal = false;

    UInt32 peHeader;
    UInt32 peHeaderSignature;
    UInt16 machine;
    UInt16 sections;
    UInt32 timestamp;
    UInt32 pSymbolTable;
    UInt32 noOfSymbol;
    UInt16 optionalHeaderSize;
    UInt16 characteristics;
    UInt16 dataDictionaryStart;
    UInt32[] dataDictionaryRVA = new UInt32[16];
    UInt32[] dataDictionarySize = new UInt32[16];

    Stream fs = new FileStream(FilePath, FileMode.Open, FileAccess.Read);
    BinaryReader reader = new BinaryReader(fs);
    fs.Position = 0x128;
    fs.Position = 0x3C;  //PE header starts at 0x3C (60), and it's a 4-byte header
    peHeader = reader.ReadUInt32();
    fs.Position = peHeader;  //move to PE header start
    peHeaderSignature = reader.ReadUInt32();

    machine = reader.ReadUInt16();
    sections = reader.ReadUInt16();
    timestamp = reader.ReadUInt32();
    pSymbolTable = reader.ReadUInt32();
    noOfSymbol = reader.ReadUInt32();
    optionalHeaderSize = reader.ReadUInt16();
    characteristics = reader.ReadUInt16();

    /*
     * Now we are at the end of the PE Header and from here, the PE optional headers start...
     * To go directly to the DataDictionary, we'll increase the stream’s current position by 96 (0x60).
     * 96 because:
     *      28 for Standard fields
     *      68 for NT-specific fields
     * From here DataDictionary starts, and it's 128 bytes.
     * DataDictionay has 16 directories in total, so 128/16 = 8.
     * So each directory is 8 bytes.
     * In that 8 bytes, 4 bytes is RVA and 4 bytes is Size.
     * The 15th directory contains the CLR header, so if it's 0, it's not a CLR file.
    */

    dataDictionaryStart = Convert.ToUInt16(Convert.ToUInt16(fs.Position) + 0x60);
    fs.Position = dataDictionaryStart;
    for (Int32 i = 0; i < 15; i++)
    {
    dataDictionaryRVA[i] = reader.ReadUInt32();
    dataDictionarySize[i] = reader.ReadUInt32();
    }
    if (dataDictionaryRVA[14] == 0)
    retVal = false;
    else
    retVal = true;

    fs.Close();

    return retVal;
}

I have experimented with processing the DLLs at application start-up, and that is still an option, but I would rather defer this if possible. It is likely that most of the time, the user will have no need to add reports, and therefore no need to suffer this processing time at all. So to force it to happen 100% of the time at startup seems counter-productive.

Another thing I find curious is that the first two DLLs processed always take a relatively large amount of time to process, compared with the rest. Usually each on the order of 150-300ms each. The first is always the longest, then the second is maybe half the first. After that, the time per DLL drops to the 30-50ms range. I have no idea how relevant this is, but it's odd and consistent.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Good job on your first question. \$\endgroup\$ – SirPython Feb 8 '16 at 20:50
  • \$\begingroup\$ Could it be done lazily? Only process it if you have to, but cache the results so it could be reused? \$\endgroup\$ – RubberDuck Feb 9 '16 at 0:25
  • \$\begingroup\$ @Heslacher I have no idea what you mean. There are no methods there that are not part of the framework. The only custom method is FileIsCLRAssembly(), and you can clearly see that it is there. \$\endgroup\$ – DonBoitnott Feb 10 '16 at 12:25
  • 1
    \$\begingroup\$ @RubberDuck This is the lazy way. The point of this method is to produce a list that provides the user with all of their available choices. Those choices could change if reports are added or removed, so the list is built anew each time. This happens only when they click the "add new" button, so I'm not sure how much further I might be able to take it. Once the assemblies have been loaded once in an app session, subsequent passes through this method are dramatically faster. I think it has something to do with .NET cheating on having seen a particular DLL before. \$\endgroup\$ – DonBoitnott Feb 10 '16 at 12:28
  • 1
    \$\begingroup\$ @RobH Still targeting .NET 3.5. \$\endgroup\$ – DonBoitnott Feb 10 '16 at 14:28
1
\$\begingroup\$

By extracting wether or not a found file is valid to a method like so

private bool IsValidReport(string file)
{
    string fileName = Path.GetFileName(x).ToLower();

    if (fileName.StartsWith("rpt_") || fileName.StartsWith("rpt~") || fileName.StartsWith("rpt_"))
    {
        return false;
    }

    return NativeMethods.FileIsCLRAssembly(fileName);
}  

and using Directory.EnumerateFiles() we will reduce the horizontal spacing by one level.

If we further continue the loop for the case that the bytes.Length == 0 we can save one level of indentation as well.

By renaming theDLL to loadedAssembly its clearer what it is about. As it seems you are taking the first type where the baseReport type is assignable from I would like to suggest using FirstOrDefault() on the returned array from GetTypes().

This altogether leads to

public void ProduceReportList()
{
    var path = Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "rpt_dll");
    var files = Directory.GetFiles(path, "*.dll", SearchOption.TopDirectoryOnly)
                .Where(file => IsValidReport(file)).ToArray();

    var reportType = typeof(baseReport);    
    foreach (var file in files)
    {
        var bytes = File.ReadAllBytes(file);
        if (bytes.Length = 0)
        {
            continue;
        }

        Assembly loadedAssembly = Assembly.Load(bytes);
        Type foundType = loadedAssembly.GetTypes()
                                       .FirstOrDefault(t => reportType.IsAssignableFrom(t));

        if (foundType == null) 
        {
            continue;
        }

        using (var report = (baseReport)Activator.CreateInstance(foundType))
        {
            if (report != null)
            {
                //Report is validated and added to a list if it passes
            }
        }
    }
}

Bear in mind the remarks section of both Directory.GetFiles() and also Directory.EnumerateFiles() which states

When you use the asterisk wildcard character in a searchPattern such as "*.txt", the number of characters in the specified extension affects the search as follows:

If the specified extension is exactly three characters long, the method returns files with extensions that begin with the specified extension. For example, "*.xls" returns both "book.xls" and "book.xlsx".

maybe it would be a good thing to change the IsValidReport() method to also check if the passed in filename ends with .dll.


Performance

Because the method can't be faster then your harddisc you should consider to use a BackgroundWorker which is started at the application start doing all the work.

I won't explicitly review the FileIsCLRAssembly() method but will add some pointers

  • method arguments should be named using camelCase casing so FilePath -> filePath
  • a FileStream should always be enclosed in a using statement so in case of an exception the stream will be properly disposed.
  • the same is true for the BinaryReader
  • a public method should always check the passed arguments before it uses them. If FilePath is null you should for instance throw an ArgumentNullException.

\$\endgroup\$

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.