Closed simu closed 8 months ago
Note that the PR has a slight performance regression (the benchmarks in #47 report a performance regression of 2-4%).
As far as I'm able to tell, this is caused mostly by the additional state that we're tracking:
1) The ResolveState
struct is larger, which makes cloning it during reference resolution more expensive
2) We introduce a bunch of extra allocations because we track the current key in a Vec<String>
-- note that tracking the current key in a String
isn't faster.
Follow-up regarding performance: I tested a bit more and have found that pre-allocating the variable-sized ResolveState
fields (seen_paths: HashSet::with_capacity()
and current_keys: Vec::with_capacity()
) doesn't seem to make a significant difference in runtime.
To improve the error messages for reference resolution failure, we add a
Vec<String>
to track the current parameter key for which we're resolving references into theResolveState
struct.We also extract the error rendering in
Token::resolve()
into methods onResolveState
, since we mostly need data fromResolveState
to render the error messages.The new error messages all print the parameter key for which we're resolving references. If applicable, the error messages also contain the full reference, and the particular key for which resolution failed.
To reduce repetition for the error handling, we also refactor
Token::resolve()
to always either interpolate or copy the Value which is the base for the key lookup. While this introduces a bunch of unnecessary calls toclone()
if we're not doing a lookup into aValue::String
orValue::ValueList
, the performance impact is negligible, and the resulting code is much less convoluted.Resolves #69
Checklist
bug
,enhancement
,documentation
,change
,breaking
,dependency
,internal
as they show up in the changelog