Use a Dictionary to optimize performance of CompiledTemplate.TryGetFormalArgument

[ndrwrbgs]

** 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's FormalArgument 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 type List<>, 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 exposing List)

[sharwell]

I'm not planning to make breaking changes in ST3 or ST4. I've considered working on a new major certain where templates are immutable objects (resolving the open concurrency issues) but no current plans to do so.

[ndrwrbgs]

Darn, so much potential in the idea of making a parser out of a template, but due to this lookup, regex and string raw manipulation out-performed the library, undoing all that parser work it did :(

List (exposed today to users as writeable) isn't sealed so I looked into just hiding inside an implementation of that to catch outside writes but it doesn't have the necessary hooks to do that.

It IS however possible to non-breaking use List's version field to on-demand know when to invalidate a cached dictionary, not sure how often users are writing to the formal arguments collection though... That code would also rely on reflection into BCL types, but hey if we can't do breaking changes sometimes other bad things are the options to choose :).