Interleaved view support, version 6#237
Draft
martinling wants to merge 23 commits intogreatscottgadgets:mainfrom
Draft
Interleaved view support, version 6#237martinling wants to merge 23 commits intogreatscottgadgets:mainfrom
martinling wants to merge 23 commits intogreatscottgadgets:mainfrom
Conversation
c429d04 to
80a47c4
Compare
This commit adds support for interleaved regions and nodes to the tree model, but does not use it yet. This commit can be tested to verify that the changes do not impact the non-interleaved case.
80a47c4 to
ad4c0a5
Compare
This file contains hidden or 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
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.
This PR supersedes #150, and contains my work in progress on my sixth attempt at merging the interleaved view. This has been an ongoing project since the very beginning of Packetry's development. It's difficult, but we're getting there.
What is an interleaved view?
The current view in Packetry is hierarchical, but not fully chronological.
If you expand a transfer, then all its transactions appear beneath it, before any other transfers -- even if the transfers actually overlap in time. The timestamp column therefore becomes out-of-order.
This hierarchical approach can be useful when trying to look at the content of a specific transfer, but also misleading, when trying to understand how different transfers are related in time.
In an interleaved view, expanding parent items always maintains chronological ordering. This means that the transactions of a transfer must appear mixed in amongst other items below. It's also possible to expand multiple transfers, and see how their transactions are interleaved in time. Here's a simple example:
Why is it hard?
In principle, it's not hard. Conceptually, it just requires sorting all rows in the tree model by timestamp and tree depth.
However, it's hard to do efficiently.
Packetry is designed from the ground up to support captures of unlimited size. It achieves this by not loading everything in advance. The tree list view you see is never fully populated. When you scroll through the view, the items visible at each position are loaded and decoded on demand. And everything is indexed and organised such that this lookup is very fast.
This is much harder for an interleaved view. Let's say you have two overlapping transfers, each with a couple million transactions, and you expand them both. And then you scroll to row 1,528,913 somewhere in the middle of the overlap. Quick, what should be there? Well, that depends on the ordering of all the transactions between the two expanded transfers.
The naive way to find the answer would be to load those several million transactions, sort them, and then take the one at that index. But that's going to slow the UI to a crawl. We need a solution that gets us there near-immediately.
And that's what this PR does, in essence. It uses an algorithm to find what should be at any given row of an interleaved view, in O(m log(n)) time, where m is the number of expanded transfers and n is the number of transactions involved.
But to reduce the problem to the point where we can apply that algorithm, we also need to do a lot of work maintaining the region map of our custom
TreeListModelimplementation, such that we always know what form of lookup we need to make for any given row.What's new in this version?
This version builds on top of the new database snapshotting feature in #234, which eliminates a whole class of bugs related to trying to update the tree view while the capture is still changing underneath.
What's the status of this?
It's usable in a lot of cases. Give it a try!
It still has some bugs though, where the code maintaining the region map miscalculates the length or offset of a region.
These miscalculations are detectable via various cross-checks (e.g. we check the total length of the region map against the expected number of rows calculated separately, and in test mode we also verify the ordering of all items that appear in the view).
Note that with snapshotting, these failures are now fully deterministic, and can be captured using the
record-ui-testfeature and turned into test cases. A currently failing test is included at the end of the branch.There are a number of speed optimisations that are not included in this version of the branch. Once the region map code is working reliably with good test coverage I will add them back in.
Additionally, the connecting lines between items are still using the same implementation as for the hierarchical view, so they don't always show all the relationships between items correctly.