tags:

views:

919

answers:

6

Hi All--

I have a VB6 application that I am converting to .net. I am doing this in phases so clients will have both VB6 and .net applications at the same. Part of the application caches ADO 2.8 COM recordsets to a table in SQL Server and retrieves them as needed. The .net application uses that same persisted recordsets. I have c# code that retrieves the persisted recordset and converts it to a dataset. My question is -- Am I doing it in the most efficient manner?

This is my code that retrieves the recordset from the database --

Stream adoStream = null;
SqlParameter cmdParameter;
SqlCommand cmd = null;
SqlDataReader dr = null;

string cmdText;
int bytesReturned;
int chunkSize = 65536;
int offSet = 0;

UnicodeEncoding readBytes;

try
{
    cmdParameter = new SqlParameter(parameterName, idParamter);

    cmdText = sqlString;

    cmd = new SqlCommand();
    cmd.CommandType = CommandType.Text;
    cmd.CommandTimeout = 0;
    cmd.CommandText = cmdText;
    cmd.Connection = this.pbiSQLConnection;
    cmd.Parameters.Add(cmdParameter);
    dr = cmd.ExecuteReader(CommandBehavior.SequentialAccess);
    dr.Read();

    if (dr.HasRows)
    {
        readBytes = new UnicodeEncoding();
        byte[] byteChunk = new byte[chunkSize];

        adoStream = new Stream();
        adoStream.Type = StreamTypeEnum.adTypeText;
        adoStream.Open(Type.Missing, ConnectModeEnum.adModeUnknown,
            StreamOpenOptionsEnum.adOpenStreamUnspecified, "", "");

        do
        {
            bytesReturned = (int)dr.GetBytes(0, offSet, byteChunk, 0,
                chunkSize);
            size += bytesReturned;
            if (bytesReturned > 0)
            {
                if (bytesReturned < chunkSize)
                {
                    Array.Resize(ref byteChunk, bytesReturned);
                }

                adoStream.WriteText(readBytes.GetString(byteChunk),
                    StreamWriteEnum.stWriteChar);
                adoStream.Flush();
            }

            offSet += bytesReturned;
        } while (bytesReturned == chunkSize);
    }
}
catch (Exception exLoadResultsFromDB)
{
    throw (exLoadResultsFromDB);
}
finally
{
    if (dr != null)
    {
        if (!dr.IsClosed)
        {
            dr.Close();
        }

        dr.Dispose();
    }

    if (cmd != null)
    {
        cmd.Dispose();
    }
}

This is the code that converts the ado stream to a datasets --

adoStream = LoadTextFromDBToADODBStream(resultID, "@result_id",
    "some sql statement", ref size);
if (adoStream.Size == 0)
{
    success = false;
}
else
{
    adoStream.Position = 0;

    DataTable table = new DataTable();
    Recordset rs = new Recordset();
    rs.Open(adoStream, Type.Missing, CursorTypeEnum.adOpenStatic,
              LockTypeEnum.adLockBatchOptimistic, -1);

    if (adoStream != null)
    {
        adoStream.Close();
        adoStream = null;
    }

    source.SourceRows = rs.RecordCount;
    table.TableName = "Source";
    source.Dataset = new DataSet();
    source.Dataset.Tables.Add(table);

    OleDbDataAdapter adapter = new OleDbDataAdapter();
    adapter.MissingSchemaAction = MissingSchemaAction.AddWithKey;
    adapter.Fill(source.Dataset.Tables[0], rs);

    if (adapter != null)
    {
        adapter.Dispose();
        adapter = null;
    }

    if (adoStream != null)
    {
        adoStream.Close();
        adoStream = null;
    }

    if (rs != null)
    {
        if (rs.State == 1)
        {
            rs.Close();
        }

        rs = null;
    }
}

Thanks all

EDIT: I added a bounty to see if anyone can make the code more efficient.

A: 

If you are saying that the entire COM recordset is being persisted to a single column in the database table as a binary object (byte array), then I don't see any way around the complexity. You have to convert the byte array into the same concrete object it was serialized from (a COM recordset), before you can manipulate it.

Charles Bretana
A: 

Yes, the steps are the only way that I can think of but is the code/process that I used the most efficient?

Dan R.
Please use the "add comment" to comment on answers instead of creating new answers. You can also edit your question.
Lars Truijens
A: 

We used Ants Profiler and it looks like the code is pretty efficient but any other insights would be appreciated.

Dan R.
A: 

Not answer your specific question....but since you are specifying efficiency, I presume you are wanting speed.
Did you think about persisting it twice, both as ADO and ADO.Net, then, the requester could retrieve the most appropriate and skip the runtime conversion (this is assuming many more reads than writes). For an extra boost, maybe store n number of datasets in memory where they can be returned instantly rather than being reloaded from the db. Again, this would only be useful depending on your specific data and requests.

tbone
We have an update application written in VB6 that will have to drastically modified (if it even can be) to save to both ADO and ADO.Net datasets or rewritten in .Net We don't see either as an alternative. Also, we only load the cached data once when needed. It is then stored in a caching service.
Dan R.
+3  A: 

Generally speaking, you aren't taking enough advantage of the using statement and handling it all yourself. Unfortunately, you are doing it the wrong way, in that if you have an implementation of IDisposable which throws an exception on the call to Dispose, the other calls to Dispose do not take place. If you use the using statement, all implementations of IDisposable.Dispose will be called, no matter how nested they are.

Let's go through the LoadTextFromDBToADODBStream first. The massive issue here is that you are sharing a connection when you shouldn't be. You should be creating the connection for your operation, using it, then closing it down. That is not the case here.

So let's assume you create your connection in a separate method, like this:

SqlConnection CreateConnection()
{
    // Create the connection here and return it.
    return ...;
}

You are also going to need the following structure to properly manage your COM references:

struct ComReference<T> : IDisposable where T : class, new()
{
    private T reference;

    public T Reference { get { return reference; } }

    public static ComReference<T> Create()
    {
        // Create the instance.
        ComReference<T> retVal = new ComReference<T>();

        // Set the reference.
        retVal.reference = new T();

        // Return.
        return retVal;
    }

    public ComReference<T> Release()
    {
        // Create a copy for return.
        // Note, this is copied on the stack.
        ComReference<T> retVal = this;

        // Set this reference to null;
        this.reference = null;

        // Return the reference.
        return retVal;
    }

    public void Dispose()
    {
        // If there is a reference, then release.
        Marshal.ReleaseComObject(reference);
    }
}

You want to manage your COM references with this so that you release them when you are done with them, not through garbage collection. COM relies on deterministic finalization, and you don't get to ignore that just because you are in .NET. The structure above leverages IDisposable (and the fact it is a structure and the nuances that come with it) to help do so in a deterministic way.

The type parameter T will be the class type that is created for COM interop, in the case of the stream, it will be ADODB.StreamClass.

Your LoadTextFromDBToADODBStream then looks like this:

ComReference<StreamClass> LoadTextFromDBToADODBStream(int idParameter,
    string parameterName, string sqlString, ref int size)
{
    int bytesReturned;
    int chunkSize = 65536;
    int offSet = 0;

    // Create the command.
    using (SqlCommand cmd = new SqlCommand())
    {
        // Set the parameters.
        cmd.CommandType = CommandType.Text;
        cmd.CommandTimeout = 0;
        cmd.CommandText = sqlString;

        // See (1).
        using (SqlConnection connection = CreateConnection())
        {
            // Set the connection on the command.
            cmd.Connection = connection;

            // Create the parameter and add to the parameters.
            SqlParameter cmdParameter = new SqlParameter(
                parameterName, idParameter);
            cmd.Parameters.Add(cmdParameter);

            // Create the reader.
            using (SqlDataReader dr = cmd.ExecuteReader(
                CommandBehavior.SequentialAccess))
            {
                dr.Read();

                // See (2)
                if (!dr.HasRows)
                {
                    // Return an empty instance.
                    return new ComReference<StreamClass>();
                }

                // Create the stream here.  See (3)
                using (ComReference<StreamClass> adoStreamClass =
                    ComReference<StreamClass>.Create())
                {
                    // Get the stream.
                    StreamClass adoStream = adoStreamClass.Reference;

                    // Open the stream.
                    adoStream.Type = StreamTypeEnum.adTypeText;
                    adoStream.Open(Type.Missing, 
                        ConnectModeEnum.adModeUnknown,
                        StreamOpenOptionsEnum.adOpenStreamUnspecified, 
                        "", "");

                    // Create the byte array.
                    byte[] byteChunk = new byte[chunkSize];

                    // See (4)
                    Encoding readBytes = Encoding.Unicode;

                    // Cycle.
                    do
                    {
                        bytesReturned = (int)dr.GetBytes(0, offSet, 
                            byteChunk, 0, chunkSize);
                        size += bytesReturned;
                        if (bytesReturned > 0)
                        {
                            if (bytesReturned < chunkSize)
                            {
                                Array.Resize(ref byteChunk,
                                    bytesReturned);
                            }

                            adoStream.WriteText(
                                readBytes.GetString(byteChunk),
                                StreamWriteEnum.stWriteChar);
                            adoStream.Flush();
                        }

                        offSet += bytesReturned;
                    } while (bytesReturned == chunkSize);

                    // Release the reference and return it.
                    // See (5).
                    return adoStreamClass.Release();
                }
            }
        }
    }
}

Notes:

  1. This is where you want to create your connection. You want to use it in a using statement because you want to be assured that your resources are cleaned up, on success or failure.
  2. It's easier to short-circuit the code here and return a new instance of ComReference<StreamClass>. When you create this, it has the effect of returning a structure which has no reference (which is what you want, as opposed to calling the static Create method).
  3. In calling the static Create method, you are creating the new instance of the ADODB.StreamClass. You want to make sure that if something goes wrong, this is disposed of upon release (since it is a COM interface implementation, and dependent on deterministic finalization).
  4. There is no need to create a new UnicodeEncoding. You can just use the Unicode property on the Encoding class to use a premade instance.
  5. In calling release, you set the reference field to null on the instance that is on the current stack, and transfer it to the ComReference<StreamClass> that is returned. This way, the StreamClass reference is still alive, and when Dispose is called on the stack variable, it doesn't pass that reference to ReleaseComObject.

Moving on to the code that calls LoadTextFromDBToADODBStream:

// See (1)
using (ComReference<StreamClass> adoStreamClass =
    LoadTextFromDBToADODBStream(resultID, "@result_id",
    "some sql statement", ref size))
{
    // Set to the class instance.  See (2)
    StreamClass adoStream = adoStreamClass.Reference;

    if (adoStream.Size == 0)
    {
        success = false;
    }
    else
    {
        adoStream.Position = 0;

        DataTable table = new DataTable();

        // See (3)
        using (ComReference<RecordsetClass> rsClass = 
        ComReference<RecordsetClass>.Create())
        {
            Recordset rs = rsClass.Reference;
            rs.Open(adoStream, Type.Missing, CursorTypeEnum.adOpenStatic,
                      LockTypeEnum.adLockBatchOptimistic, -1);

            if (adoStream != null)
            {
                adoStream.Close();
                adoStream = null;
            }

            source.SourceRows = rs.RecordCount;
            table.TableName = "Source";
            source.Dataset = new DataSet();
            source.Dataset.Tables.Add(table);

            // See (4)
            using (OleDbDataAdapter adapter = new OleDbDataAdapter())
            {
                adapter.MissingSchemaAction = 
                    MissingSchemaAction.AddWithKey;
                adapter.Fill(source.Dataset.Tables[0], rs);
            }
        }
    }
}
  1. This is going to receive the return value of the call to Release in LoadTextFromDBToADODBStream. It will contain the live reference to the ADODB.Stream created there, and the using statement will guarantee it is cleaned up when the scope is left.
  2. As before, this makes it easier to reference the direct reference, instead of always having to call adoStreamClass.Reference.<method>
  3. Using another ComReference, this time ComReference<RecordsetClass>.
  4. Let the compiler do the dirty work for you.

In using the using statement more, you can clean up a lot of the code that was making it very difficult to read. Also, in general, you were cleaning up some resource issues that would have cropped up in the face of exception, as well as handled COM implementations that were not being disposed of correctly.

casperOne
A: 

The code looks pretty efficient in general. While I agree with the need to have generalized COM handling routines (just for consistency, if nothing else), I don't know what extra performance you'll wring from his recommendation not to reuse the db connection.

The only thing I'd worry about is that you are using ADO Stream. This particular object has a small side-effect of eating memory like a kid in a candy store. I'd check out this article ( http://www.vbrad.com/article.aspx?id=12 ) and make sure that your code does not have this problem.

AngryHacker