PERF: Reduce boxing allocations in GetScalarValue#2938
Conversation
| private static bool LoadBool(JsonNode node) | ||
| { | ||
| var value = node.GetScalarValue(); | ||
| return value is not null ? bool.Parse(value) : null; |
There was a problem hiding this comment.
here we're changing the behaviour. We're assigning false to values that are nullable instead of null, aren't we?
There was a problem hiding this comment.
This is used for $vocabulary. https://json-schema.org/draft/2020-12/json-schema-core#section-8.1.2
The values of the object properties MUST be booleans.
There was a problem hiding this comment.
ah this is only used at that place (vocab), I thought it was used multiple places and changed the semantics by replacing the default from null to false. Let's drop the method all together and inline it (both for 3.1 and 3.2)
| // It's much more efficient to call scalarNode.TryGetValue<string> than to call scalarNode.GetValue<object>() and then convert to string. | ||
| // When asking for "object" type, if scalarNode is JsonValueOfElement (internal type in STJ), we will get back | ||
| // a boxed JsonElement (paying the cost of unnecessary boxing allocation), and we then call JsonElement.ToString. | ||
| // So, we first check if we can get the string value directly, and only if that fails, we fallback to the expensive code. |
There was a problem hiding this comment.
I think that one reason why we had to do that was to avoid mangling like default, numbers with formats, etc... While I do appreciate the performance improvements, I don't want it to come at the price of a regression.
There was a problem hiding this comment.
TryGetValue<string> is never going to throw or do something strange. It will return true if the node really represents a string. For example, if you have any numbers in JSON like "maximum": 10, TryGetValue<string> will return false because the node doesn't represent a string. And then we fallback to the original implementation.
I done it this way explicitly because I saw that some numerics are read raw as strings that way.
There was a problem hiding this comment.
This is what I'm referring to c17a87e
I think the object convert was introduced to avoid formatting issues for defaults and other properties when using types like dates and such.
| { | ||
| return (int)longValue; | ||
| } | ||
| else if (scalarNode.TryGetValue<decimal>(out var decimalValue)) |
There was a problem hiding this comment.
Decimal in most cases will be coming through the YML library being used to convert YML to JSON.
I'm not sure if we can get other numeric types but I haven't seen it when trying to run the test suite. However, to reduce any risks, I kept the GetValue<object> fallback.
Edit: It's not the library. It's our code to handle YML. I tried to adjust it so that it handles numeric in a more correct way.
There was a problem hiding this comment.
Pull request overview
This PR optimizes scalar extraction during OpenAPI document deserialization to reduce allocations (notably avoiding boxed JsonElement from GetValue<object>()) and centralizes scalar parsing into typed helpers for bool/int/uint values.
Changes:
- Optimizes
JsonNodeHelper.GetScalarValue()to tryTryGetValue<string>first, reducing boxing allocations. - Adds typed scalar helpers (
GetScalarBoolValue,GetScalarIntValue,GetScalarUIntValue) and updates V2/V3/V31/V32 deserializers to use them. - Simplifies boolean handling across multiple deserializers by replacing repeated
GetScalarValue()+bool.Parse(...)patterns.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.OpenApi/Reader/JsonNodeHelper.cs | Adds typed scalar helpers and optimizes string scalar extraction to avoid boxing. |
| src/Microsoft.OpenApi/Reader/V32/OpenApiV32Deserializer.cs | Switches internal bool scalar loading to typed helper. |
| src/Microsoft.OpenApi/Reader/V32/OpenApiSecuritySchemeDeserializer.cs | Uses typed bool scalar helper for deprecated. |
| src/Microsoft.OpenApi/Reader/V32/OpenApiSchemaDeserializer.cs | Uses typed bool/int/uint helpers for many scalar schema fields; updates $vocabulary mapping. |
| src/Microsoft.OpenApi/Reader/V32/OpenApiRequestBodyDeserializer.cs | Uses typed bool scalar helper for required. |
| src/Microsoft.OpenApi/Reader/V32/OpenApiParameterDeserializer.cs | Uses typed bool scalar helper for multiple parameter flags. |
| src/Microsoft.OpenApi/Reader/V32/OpenApiOperationDeserializer.cs | Uses typed bool scalar helper for deprecated. |
| src/Microsoft.OpenApi/Reader/V32/OpenApiHeaderDeserializer.cs | Uses typed bool scalar helper for multiple header flags. |
| src/Microsoft.OpenApi/Reader/V32/OpenApiEncodingDeserializer.cs | Uses typed bool scalar helper for encoding flags. |
| src/Microsoft.OpenApi/Reader/V31/OpenApiXmlDeserializer.cs | Uses typed bool scalar helper for XML flags. |
| src/Microsoft.OpenApi/Reader/V31/OpenApiV31Deserializer.cs | Switches internal bool scalar loading to typed helper. |
| src/Microsoft.OpenApi/Reader/V31/OpenApiSecuritySchemeDeserializer.cs | Uses typed bool scalar helper for x-oai-deprecated. |
| src/Microsoft.OpenApi/Reader/V31/OpenApiSchemaDeserializer.cs | Uses typed bool/int/uint helpers for many scalar schema fields; updates $vocabulary mapping. |
| src/Microsoft.OpenApi/Reader/V31/OpenApiRequestBodyDeserializer.cs | Uses typed bool scalar helper for required. |
| src/Microsoft.OpenApi/Reader/V31/OpenApiParameterDeserializer.cs | Uses typed bool scalar helper for multiple parameter flags. |
| src/Microsoft.OpenApi/Reader/V31/OpenApiOperationDeserializer.cs | Uses typed bool scalar helper for deprecated. |
| src/Microsoft.OpenApi/Reader/V31/OpenApiHeaderDeserializer.cs | Uses typed bool scalar helper for multiple header flags. |
| src/Microsoft.OpenApi/Reader/V31/OpenApiEncodingDeserializer.cs | Uses typed bool scalar helper for encoding flags. |
| src/Microsoft.OpenApi/Reader/V3/OpenApiXmlDeserializer.cs | Uses typed bool scalar helper for XML flags. |
| src/Microsoft.OpenApi/Reader/V3/OpenApiSecuritySchemeDeserializer.cs | Uses typed bool scalar helper for x-oai-deprecated. |
| src/Microsoft.OpenApi/Reader/V3/OpenApiSchemaDeserializer.cs | Uses typed bool/int/uint helpers for many scalar schema fields (incl. exclusives/nullable/contains). |
| src/Microsoft.OpenApi/Reader/V3/OpenApiRequestBodyDeserializer.cs | Uses typed bool scalar helper for required. |
| src/Microsoft.OpenApi/Reader/V3/OpenApiParameterDeserializer.cs | Uses typed bool scalar helper for multiple parameter flags. |
| src/Microsoft.OpenApi/Reader/V3/OpenApiOperationDeserializer.cs | Uses typed bool scalar helper for deprecated. |
| src/Microsoft.OpenApi/Reader/V3/OpenApiHeaderDeserializer.cs | Uses typed bool scalar helper for multiple header flags. |
| src/Microsoft.OpenApi/Reader/V3/OpenApiEncodingDeserializer.cs | Uses typed bool scalar helper for encoding flags. |
| src/Microsoft.OpenApi/Reader/V2/OpenApiXmlDeserializer.cs | Uses typed bool scalar helper for XML flags. |
| src/Microsoft.OpenApi/Reader/V2/OpenApiSchemaDeserializer.cs | Uses typed bool/int/uint helpers for many scalar schema fields (incl. exclusives/nullable/contains). |
| src/Microsoft.OpenApi/Reader/V2/OpenApiParameterDeserializer.cs | Uses typed bool/int helpers for parameter/schema scalar fields. |
| src/Microsoft.OpenApi/Reader/V2/OpenApiOperationDeserializer.cs | Uses typed bool scalar helper for deprecated. |
| src/Microsoft.OpenApi/Reader/V2/OpenApiHeaderDeserializer.cs | Uses typed bool/int helpers for header/schema scalar fields. |
| // JsonNode.Parse will create a JsonValue that is suitable for representing any numeric value (it's wrapping JsonElement). | ||
| // So, if we call '.TryGetValue<int>' on it and the underlying value can be represented as int, it will succeed. | ||
| ScalarStyle.Plain when decimal.TryParse(yaml.Value, NumberStyles.Float, CultureInfo.InvariantCulture, out var d) => (JsonNode.Parse(yaml.Value) as JsonValue) ?? JsonValue.Create(d), | ||
| ScalarStyle.Plain when bool.TryParse(yaml.Value, out var b) => JsonValue.Create(b), |
| public static bool GetScalarBoolValue(this JsonNode? node) | ||
| { | ||
| var scalarNode = node is JsonValue value ? value : throw new OpenApiException("Expected scalar value."); | ||
| return scalarNode.GetValue<bool>(); | ||
| } |
| public static int GetScalarIntValue(this JsonNode? node) | ||
| { | ||
| var scalarNode = node is JsonValue value && value.GetValueKind() == JsonValueKind.Number ? value : throw new OpenApiException("Expected numeric scalar value."); | ||
|
|
||
| if (scalarNode.TryGetValue<int>(out var intValue)) | ||
| { | ||
| return intValue; | ||
| } | ||
|
|
||
| return Convert.ToInt32(scalarNode.GetValue<object>()); | ||
| } |
| public static uint GetScalarUIntValue(this JsonNode? node) | ||
| { | ||
| var scalarNode = node is JsonValue value && value.GetValueKind() == JsonValueKind.Number ? value : throw new OpenApiException("Expected numeric scalar value."); | ||
| if (scalarNode.TryGetValue<uint>(out var uintValue)) | ||
| { | ||
| return uintValue; | ||
| } | ||
|
|
||
| return Convert.ToUInt32(scalarNode.GetValue<object>()); | ||
| } |
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
GetScalarValuecan be dealing with a JsonNode that's wrapping a JsonElement. DoingGetValue<object>will return a boxed JsonElement, which is unnecessary allocation. Then we could also force creating a string when we deal with boolean in many cases.This PR extracts a separate helper for bool values, which is more efficient and is more straightforward to use.
And for the string-based GetScalarValue, try to get the value as string right away first to avoid JsonElement boxing.