-
Notifications
You must be signed in to change notification settings - Fork 32
Optimize applies_to parsing #1942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8c0dff1 to
39a7320
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes the parsing of applies_to YAML definitions by introducing a centralized parser with memoization capabilities. The change eliminates code duplication and improves performance by caching parsed results.
Key Changes:
- Created a new
ApplicableToParserclass that provides centralized YAML parsing with memoization - Replaced direct
YamlSerialization.Deserializecalls across multiple files with the new parser - Updated test expectations to reflect changes in HTML output formatting
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Elastic.Documentation/AppliesTo/ApplicableToParser.cs | New parser class with memoization using ConcurrentDictionary |
| src/Elastic.Markdown/Myst/Roles/AppliesTo/AppliesToRole.cs | Updated to use centralized parser instead of direct deserialization |
| src/Elastic.Markdown/Myst/Directives/DirectiveHtmlRenderer.cs | Replaced local parsing method with centralized parser |
| src/Elastic.Markdown/Myst/Directives/AppliesSwitch/AppliesSwitchBlock.cs | Simplified sync key generation using new parser |
| src/Elastic.Markdown/Myst/CodeBlocks/EnhancedCodeBlockParser.cs | Updated applies_to directive processing to use new parser |
| tests/Elastic.Markdown.Tests/Directives/ApplicabilitySwitchTests.cs | Added import for new AppliesTo namespace |
| tests/authoring/Inline/AppliesToRole.fs | Updated test expectations for HTML output format changes |
6c8a96c to
b41dcd3
Compare
| /// </summary> | ||
| public static class ApplicableToParser | ||
| { | ||
| private static readonly ConcurrentDictionary<string, ApplicableTo?> ParsedCache = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conflicted about memoization here.
Deserialization should be fast (the serialization routines get source generated at build time) and these objects are typically short lived so should be GC'd away quickly too.
This does keep fewer instances around but I wonder how many cache hits we actually get with folks writing the yaml in different orders.
Applicability.TryParse() might be worth memoizing since it has a much higher chance of hitting the cache. But even there there are so many version lifecycle permutations around memoizing might eat up more memory then its worth easing GC pressure.
Changes
Extract
applies_toyaml parsing into its own class and reuse it in all the places.Also, add memoization so we don't parse the same input over and over again.