Optimize Record._elements to not duplicate VectorMap if possible

[jackkoenig]

The code is a little janky, but it is a massive reduction in memory use.

For a typical Bundle with 7 UInt fields, this reduces the memory footprint from 1992 bytes to 1552 bytes (the duplicate VectorMap of only 7 elements is 440 bytes!), a 22% reduction. Combined with my other recent performance improvement PRs (#4251, #4252, #4253 but excluding #4242), this 1992 bytes is reduced to 1424 for a total reduction of 28.5% memory use for this typical Bundle with 7 UInt fields.

I suspect we can do better by getting rid of any Map here at all and instead just use an Array[Data] (where the String names of the fields are stored in the children Data themselves as they are already stored there). Whereas the VectorMap of 7 elements is 440 bytes, an Array of 7 elements is 48 bytes.

That being said, this is a much simpler change that imparts a huge benefit so is worth it in the meantime.

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• Performance improvement

Desired Merge Strategy

• Squash

Release Notes

This reduces memory use of a typical bundle by 20%.

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
• Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[mwachs5]

is there a reason why this has to be done with a var and also why we aren't just doing it something like:

def makeNewMap(newNames: Seq[String) =       newNames.view
        .zip(originalElements)
        .map { case (name, (_, data)) => name -> data }
        .to(VectorMap) // VectorMap has O(1) lookup whereas ListMap is O(n)

val newElements = originalElements match {
  case original: VectorMap[_, _] => {
    val newNames = ...
    val allNamesUnchanged = newNames == origNames
    if (allNamesUnchanged) original else makeNewMap(newNames)
  case _ => makeNewMap(newNames)
}

...? I guess you are trying to only iterate over it once ...?

[jackkoenig]

I guess you are trying to only iterate over it once ...?

Correct, I'm trying to minimize the total number of traversals

[adkian-sifive]

LGTM, the changes make sense