feat: Further improve performance of stack function #1847
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
December 31, 2024 03:05
The return value of _traceVisible is frequently passed to performStacking. performStacking performs better based on how close the ordering of the input array is to ascending start times. Prior to this change, half of _traceVisible's output would be ordered consistently with the input order, but half would be reversed. After the change, the output order is consistent with the input order. Testing in Firefox on a 6,000 item dataset with no custom order function showed that this change reduced stacking time by a modest ~5%. When a custom order function was provided, no performance improvement was measured.
When I wrote visjs#1281 I was under the impression that calling `Array.slice` and then using the resulting array in a read-only capacity would be optimised by JS engines to use an offset/crop into the same memory as the original array, similar to how Node's Buffer.subarray works. I have since discovered that this is not the case: in both Chrome & Firefox,`Array.slice` always carries the cost of shallow-copying the array. Avoiding the use of `slice` significantly improves stacking performance with large amounts of items. (On a dataset of 6,000 items, this reduces stacking time by 5-20%!)
performStacking performs better based on how close the ordering of the input array is to ascending start times. The input to performStacking is derived from the Group.visibleItems array. This array is sometimes sorted by start time (for example, when items are added to it via the _traceVisible method and this method was given orderedItems.byStart as an argument). But other times (such as when _traceVisible is given orderedItems.byEnd, or when the _checkIfVisible method is called) items are not added to the array in any useful order. This change keeps the visibleItems array sorted by start time. In a dataset of 6,000 items with no custom ordering, I measured a 1-4% reduction in stacking time from this change.
This fixes several bugs and oversights in the stacking algorithm. None of these affected accuracy but they did affect performance. - Since the horizontalOverlapEndIndex value is precise (unlike horizontalOverlapStartIndex, which is a conservative approximation) we don't need to check each item's start in the `filterBetween` call. - previousEnd was never set, which meant that the horizontalOverlapEndIndex would never be moved backwards. (This was not noticed earlier because the previous oversight hid the resulting issues.) - There was a capitalisation typo (`horizontalOVerlapEndIndex`) but it was in code rendered unreachable by the previous bug. - There was an off-by-one error where inserting an item into an array made the tracked indexes incorrect. The impact of this was also not noticed due to the previous bugs. Overall this commit on average reduces stacking time by 4% when a custom order function is in use (tested with a set of 6,000 items). There is no change when a custom order function is not provided.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm currently writing a blog post about the optimisations in my previous PR #1281. In the process of writing this blog post, I have identified several further minor performance improvements. During testing on a dataset of 6,000 items, these reduced time spent in the stacking function by an average of 15%.
This PR also partially exists to salvage my ego, because my benchmarks revealed that the fancy algorithm I wrote in #1281 actually performs worse (in some circumstances) than a naive algorithm I used for comparison. With the changes in this PR, the algorithm I worked hard on now confidently outperforms the naive algorithm in all circumstances I could think to test.