avoiding allocations in FieldCollectorParameters and MergedField#2074
Conversation
|
For MergedField:
|
ArrayList allocated in the Builder never escapes the builder, so it is possible to avoid one copy in the private ctor
9e6ff2a to
ee6ed0d
Compare
| public Builder fields(List<Field> fields) { | ||
| this.fields = fields; | ||
| this.fields.addAll(fields); | ||
| return this; |
There was a problem hiding this comment.
used only in tests, could be a breaking change.
Maybe this is the root cause of the additional copy that I've just removed in the constructor.
| public Builder variables(Map<String, Object> variables) { | ||
| this.variables.putAll(variables); | ||
| this.variables = variables; | ||
| return this; |
There was a problem hiding this comment.
This is a tricky one - generally we try for more immutable classes and hence by "copying" we gain some control in mutable Java land
However as this class is repeatedly called, we probably need to change this general pattern if we see evidence that it costs us.
There was a problem hiding this comment.
yes, I'm not sure too. I did that because the class is marked as @internal (so we may have more freedom) but I know that it is against the pattern mutable/builder -> immutable/built object.
It looks like it is always called with ExecutionContext data, that is unmodifiable.
|
Thank you @bbakerman! I just integrated this new version and run it under the memory profiler. Results are a bit better (1-2%): |
ExecutionContext uses immutable maps for variables and fragments, that could be shared freely.
And, since this is an internal class, we could change a bit the contract and avoid some allocations of LinkedHashMap: