Skip to content

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Apr 11, 2021

This moves the netcore SqlStatistics implementation to shared and applies some minor style changes.

It also adds a new ref struct called ValueSqlStatisticsScope and a factory method on SqlStatistics.TimedScope to create them. This will enable the use of statistics to be made less verbose. From

override public string GetDataTypeName(int i)
{
    SqlStatistics statistics = null;
    try
    {
        statistics = SqlStatistics.StartTimer(Statistics);
        CheckMetaDataIsReady(columnIndex: i);

        return GetDataTypeNameInternal(_metaData[i]);
    }
    finally
    {
        SqlStatistics.StopTimer(statistics);
    }
}

to:

override public string GetDataTypeName(int i)
{
    using (SqlStatistics.TimedScope(Statistics))
    {
        CheckMetaDataIsReady(columnIndex: i);

        return GetDataTypeNameInternal(_metaData[i]);
    }
}

The struct is a ref struct for two reasons. 1) it prevents it being included in async closures which is good because async implementations don't start and end in the same method in this library implementation most of the time. 2) is keeps it on the stack in the same way that the original variable was used again preventing leakage.

The reason for the struct being ref and not implementing IDispose despite being used in a using block are noted at the type definition. ref structs can't implement interfaces because interface dispatch requires the variable to be boxed and ref types can't be. The compiler uses pattern matching on ref structs and the dispose interface method so that it can be used in using blocks idiomatically.

The changes to consumers to use the new scope will be done in a separate PR.

@cheenamalhotra cheenamalhotra merged commit 91c5844 into dotnet:main May 7, 2021
@Wraith2 Wraith2 deleted the combine19 branch May 7, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants