SqlMetadataPrivFlags remove Enum.HasFlag in favor of bitwise operations

[cmeyertons]

Noticed this in the profiler -- IsNullable was allocating the enum struct because of Enum.HasFlag

There's pretty wide spread usage of Enum.HasFlag throughout the repo, but this is the only one occurring in the SqlBulkCopy critical path.

[cheenamalhotra]

Please also update the same for other flags while you're at it.
Search for "IsDifferentName" in the same class, there're 6 more of these.

[Wraith2]

Netcore or netfx? because in netcore I was specifically advised to use Enum.HasFlag because it's intrinsic and the compiler should recognise it and emit good assembly.

[roji]

Yeah, Enum.HasFlag is indeed recommended in .NET Core. Anyone using .NET Framework at this point can't really care that much about high-perf, given the huge gaps between Framework and Core.

[cmeyertons]

@roji @Wraith2 I'm seeing the allocations from SqlMetadataPriv.GetNullable (Enum.GetFlag) occur in the JetBrains DotTrace TimeLine Profiler while running netcoreapp3.1-Release. It's normally pretty spot on about tracking memory allocations

I did a quick benchmark and you guys are correct - it does not seem to be allocating in my corporate .NET 5 application:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18363.1500 (1909/November2018Update/19H2)
Intel Xeon E-2176M CPU 2.70GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=5.0.202
  [Host] : .NET Core 5.0 (CoreCLR 5.0.521.16609, CoreFX 5.0.521.16609), X64 RyuJIT

Job=InProcess  Toolchain=InProcessEmitToolchain  

Method	Effect	Mean	Error	StdDev	Median	Rank	Gen 0	Gen 1	Gen 2	Allocated
Control	First	0.0002 ns	0.0010 ns	0.0009 ns	0.0000 ns	1	-	-	-	-
Bitwise	First	0.0145 ns	0.0173 ns	0.0144 ns	0.0127 ns	1	-	-	-	-

I also checked .NET core 3.1 and results were good there too.

I guess i'm still confused why it's showing up in netcoreapp3.1-Release DotTrace profiler

[Wraith2]

I've thought this thought a bit. The PR's when I was changing this are when I was working on System.Data.SqlClient in the corefx repository and at that time the only target was netcore and it made sense to use HasFlag because it's simpler and had been fixed for netcore not to have bad codegen. Working on Microsoft.Data.SqlClient is different because we've got the netfx codebase to think about and one of the long term goals here is to unify as much code as possible between the two so that netfx perf improves wherever possible alongside the direct target of netcore.

Codegen on netcore between the two is similar enough that it's unlikely to be the cause of a major performance disruption to use masking on netcore.

So HasFlag was the correct choice when it was made but direct masking is now the correct choice and I think that should be the overall policy in this codebase. I'll try to incorporate that into my future PR's as well.

[Wraith2]

LGTM

[cheenamalhotra]

Please also update the same for other flags while you're at it.
Search for "IsDifferentName" in the same class, there're 6 more of these.

@cmeyertons would you be able to address this small request soon?

[Wraith2]

I can pick up that change in a separate PR if that's easier.

[cmeyertons]

@cheenamalhotra @Wraith2 earliest I can get back to this is Monday :/

[cheenamalhotra]

FYI - I've updated the PR with additional changes to unblock from review.

[DavoudEshtehari]

Could you update the Updateability to use the related enum member instead of 0x3?

        public byte Updatability
        {
            get => (byte)(flags & _SqlMetadataFlags.IsUpdatableMask);
            set => flags = (_SqlMetadataFlags)((value & (byte)_SqlMetadataFlags.IsUpdatableMask) | ((int)flags & (int)~_SqlMetadataFlags.IsUpdatableMask));
        }