Improved execution field merging #2019
Conversation
| f._condition.isEmpty || f._condition.get.contains(typeName) | ||
|
|
||
| def mergeFields(fields: List[Field]) = { | ||
| val map = new java.util.LinkedHashMap[String, Field]((fields.size / 0.75d).toInt + 1) |
There was a problem hiding this comment.
Is this a known formula? Or just an estimate?
There was a problem hiding this comment.
Would be good to explain what this is in a comment
There was a problem hiding this comment.
The exact formula would be Math.ceil(size / 0.75) but this is close enough I believe. I can use that instead if you prefer.
The Map resizes when the number of items in it exceed the capacity * loading factor (defaults to 0.75). Using this formula guarantees that we have adequate capacity so that the Map won't need to resize. In JDK17+ there is a method that allows you to create the Maps taking the expected number of items as a parameter (instead of using the initial size) and it used this formula.
I'll add a comment for it.
There was a problem hiding this comment.
I forget is scala list's size O(n) or is it stored on concat?
There was a problem hiding this comment.
It's O(n), although given that this List only contains the queried fields and that this method is only run for objects with multiple fragments, I think the overall impact of counting the number of elements in this list is extremely small
| val condition = head._condition | ||
| var remaining = tail | ||
| while (!remaining.isEmpty) { | ||
| if (remaining.head._condition != condition) return false |
There was a problem hiding this comment.
I was gonna suggest tail recursion but this will probably iterate tighter
There was a problem hiding this comment.
Hmm I think the compiler will rewrite the tailrec method likely to this implementation. I'll check, if it's the same I'll use tailrec
There was a problem hiding this comment.
Looking at the generated code, it looks like @tailrec uses Java labels in an infinite while loop with return statements, whereas this one uses the !remaining.isEmpty method as a condition to terminate the while loop. Not sure which one would be more performant in theory
There was a problem hiding this comment.
Yeah we're probably getting into a grey area of compiler optimization. I think the conditionless while might be able to skip a branch if equal instruction, though for modern chips branches aren't really performance killers anymore
| * | ||
| * NOTE: This method is the same as java.util.HashMap.calculateHashMapCapacity on JDK19+ | ||
| */ | ||
| private def calculateMapCapacity(nMappings: Int): Int = |
There was a problem hiding this comment.
After merging this you can use it in the other PR
In many cases, there's would be only a single condition on fields (either None or Set(Something)). In these cases, the field merging has already been handled within
Field.apply, which means we don't need to do it again.For these cases, we up to 15% improvement in execution time depending on the number of fields in the object