OData / odata.net

ODataLib: Open Data Protocol - .NET Libraries and Frameworks
https://docs.microsoft.com/odata
Other
685 stars 349 forks source link

Implement cleaner approach to use of character array pool in JsonWriter #2881

Open gathogojr opened 6 months ago

gathogojr commented 6 months ago

The JsonWriter class contains a public property named ArrayPool of type ICharArrayPool that is intended to support use of character array pools. However, that property is not defined in the IJsonWriter interface and that contributes to code smell like the following:

if (jsonWriter is JsonWriter defaultJsonWriter)
{
    defaultJsonWriter.ArrayPool = writerSettings.ArrayPool;
}

https://github.com/OData/odata.net/blob/57572612f1ff833d395b44e6b2c498d7d0d31890/src/Microsoft.OData.Core/JsonLight/ODataJsonLightOutputContext.cs#L1017

We should consider a redesign to have a cleaner approach. Maybe the ArrayPool could be a property of the DefaultJsonWriterFactory instead and the factory could pass it down to the JsonWriter.

Alternatively, like @habbes suggested, we should get rid of the property and just use an ArrayPool<T>.Shared internally when necessary.

Assemblies affected

Microsoft.OData.Core 7.20.0

habbes commented 6 months ago

I would suggest that we remove the property altogeher. I suggest that we just use an ArrayPool<T>.Shared internally when we need one.

corranrogue9 commented 6 months ago

From our triage meeting, let's remove the configurability and use the shared singleton array pool instead. I will add more comments later to capture our full discussion. Let's double check with substrate to potentially do an A/B test on the perf.