Speed up nml import for nmls with many trees

[daniel-wer]

I noticed that the NML import for NMLs with many trees (20,000+) was very slow and had a look. Turns out I introduced this regression in #4541 by repeatedly calling the getMaximumTreeId function when importing trees. The getMaximumTreeId function was also rather slow as it created a potentially large array everytime it was called. I replaced that with a simple reduce (10x speedup for tracings with >20,000 trees) and also made sure to only call the getMaximumTreeId function when it is necessary (~100x speedup during nml import).

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• Unzip and import synapses_0.80.zip in an existing tracing. The import should be fast and not take multiple minutes.

---

• Updated (unreleased) changelog
• Updated (unreleased) migration guide if applicable
• Updated documentation if applicable
• Adapted wk-connect if datastore API changes
• Needs datastore update after deployment
• Ready for review

[philippotto]

Did you also compare this to a simple for-loop? I think, I remember a case where this was another large performance boost. But maybe only because the aggregator was an object in that particular case. For a simple number it might be without benefit. Maybe worth to test, though.

Also: Did you compare line 76 to Math.max(tree.treeId, maxId)? If your current way is faster, that's fine, but maybe there is no difference. In that case, I'd prefer the math.max version :)

[daniel-wer]

Very good remarks! I did some further testing and also noticed that I worked with a trees array (instead of an object) in my benchmarks 😕 . When using an object there doesn't seem to be a significant speedup over the existing lodash method using reduce or a foor lop (because Object.keys/values needs to be called). I did not spent much more time investigating because not calling getMaximumTreeId 20,000+ times provided the much greater speedup and optimizing the getMaximumTreeId function seems more like premature optimization at this point :)

[philippotto]

When using an object there doesn't seem to be a significant speedup over the existing lodash method using reduce or a foor lop (because Object.keys/values needs to be called).

Did you notice the "not significant speed ups" while micro benchmarking getMaximumTreeId or while e2e-benchmarking the entire nml import? Since getMaximumTreeId is also used in the rest of WK core, I'd find it worth it to swap the implementation to reduce/for-loop in case there's at least some speed-up factor :)

[daniel-wer]

I micro-benchmarked getMaximumTreeId and smaller speedups are noticeable only when calling getMaximumTreeId >10,000 times for a tracing with at least 20,000 trees. As we don't call it nearly that often (after my fix in this PR), I'm not convinced we need to optimize the function.

[philippotto]

Cool, totally agree then 👍

[philippotto]

Very cool that you spent the time to find this culprit :)