Use a Dictionary to optimize performance of CompiledTemplate.TryGetFormalArgument #122
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
** NOT SAFE AS IS **
This review is mostly to start the discussion and get a feel for how open the maintainers are to breaking changes
Problem
During profiling of
Template.Render
,CompiledTemplate.TryGetFormalArgument
was showing up an alarming amount. It turns out this is because it's doing some LINQ over it'sFormalArgument
collection to find the matching arguments by name.Note
When I looked at the Java implementation, FormalArguments is a Map (which is ~= Dictionary in C#)
Solution
Store the formal arguments by their names as keys
Problems with review as-is
Currently,
CompiledTemplate
exposes FormalArguments as typeList<>
, which is an editable type.Therefore, a user can today go
(Template template).impl.FormalArguments.Add()
instead of the API desired
(Template template).impl.AddArgument()
This makes it impossible to keep a separate argument synchronized. Also it makes it impossible to just store the value as, say, a Dictionary and do
dictionary.Keys.ToList()
since that would make a copy and break users who were doing.FormalArguments.Add()
If we are willing to take that break, then I would revise this review to just making FormalArguments directly a dictionary. (The complication with that is the arguments are strongly-ordered by the API, and
Dictionary
is unordered. There is likely some class somewhere that can store both Dictionary and original ordering -- but regardless we'd need to stop exposingList
)