i#7731: Optimize reuse distance collection using splay tree by artur-abd · Pull Request #7732 · DynamoRIO/dynamorio

artur-abd · 2025-11-26T16:37:11Z

Improve reuse distance using splay tree instead of linked list. Splay tree have asymptotic O(log n) amortizated time for all operations instead of O(n / const) for linked list with skip list.
Perfomance improve achives in avarage 10-40% based on different benchmarks.

Fixes #7731

Improve reuse distance using splay tree instead of linked list. Splay tree have asymptotic O(log n) amortizated time for all operations instead of O(n / const) for linked list with skip list. Perfomance improve achives in avarage 10-40% based on different benchmarks. Fixes DynamoRIO#7731

derekbruening · 2025-11-28T16:37:04Z

Thank you for contributing. Please mark for review when ready. Also, please don't force-push a shared branch: see https://dynamorio.org/page_code_reviews.html about squash-and-merge, where the final merge commit message comes from, and how force pushes break the review process. Use separate incremental commits with commit messages that describe what each one is doing (and not just repeating the merge message).

Fixes description for line_ref_splay_t and it's methods Fixes: DynamoRIO#7731

artur-abd · 2025-12-01T14:04:53Z

Thanks for your comments. PR is ready for review.
Tests for ci-windows / vs2022-64 (pull_request) is failed due to a timeout in other modules. Self triggered tests passed.

Copilot

Pull request overview

Optimizes drcachesim’s reuse-distance collection by replacing the linked-list (+skip list) data structure with a splay tree to reduce per-access overhead.

Changes:

Replaced the internal reuse-distance tracking structure with a splay tree implementation.
Updated reuse-distance tool/shard wiring to use the new node type and tree container.
Added unit tests for the splay tree and marked skip-list-related CLI options as deprecated.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
clients/drcachesim/tools/reuse_distance.h	Replaces list/skip-list logic with a splay tree, adds splay operations and subtree-size tracking.
clients/drcachesim/tools/reuse_distance.cpp	Updates shard initialization and reuse-distance processing to use the new node/tree types.
clients/drcachesim/tests/reuse_distance_test.cpp	Adds a dedicated splay-tree correctness/invariant test.
clients/drcachesim/common/options.h	Renames the skip-list distance option variable to reflect deprecation.
clients/drcachesim/common/options.cpp	Deprecates skip-list-related option help text (and keeps CLI flag name).
clients/drcachesim/analyzer_multi.cpp	Updates option wiring to use the renamed deprecated option variable.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

artur-abd · 2026-01-23T17:01:22Z

@abhinav92003, thanks for review.
I've added tests and marked the skip list options as deprecated.
However, I'm not sure that extracting this splay tree implementation into ext is a good idea, for the reason I mentioned above.

artur-abd · 2026-03-20T08:44:47Z

@abhinav92003
Hi, Any more questions for the PR?
I haven't applied a couple of changes you suggested before because I'm not sure the software design would be well. Please look at the detailed description in the commented answer. Let's discuss it in detail if you disagree.

abhinav92003 · 2026-03-20T14:55:44Z

@abhinav92003 Hi, Any more questions for the PR? I haven't applied a couple of changes you suggested before because I'm not sure the software design would be well. Please look at the detailed description in the commented answer. Let's discuss it in detail if you disagree.

Ack. I'll review the new changes.

abhinav92003 · 2026-03-24T14:52:48Z

            return 0;
+        splay(ref);
+        // Get the reuse distance of ref.
+        int64_t dist = get_size(ref->left);


get_size(ref->left) is the reuse distance of ref only because we did the splay operation on ref before this correct? Add comment.

Yes, after the splay operation, ref becomes the root of the splay tree, and the index of these elements is exactly equal to the size of the left subtree.

Adding this to the code comment would be helpful to a future reader.

abhinav92003 · 2026-03-24T14:54:34Z

+    struct line_ref_node_t *right;  // the right child of the node
+    struct line_ref_node_t *parent; // the parent of the node
+    uint64_t size;                  // Size of the subtree
+    uint64_t time_stamp;            // the most recent reference time stamp on this line


The splay tree maintains the ordered binary tree invariant on time_stamp right (larger timestamps on the left subtree), right? Add to comment.

The splay tree maintains this invariant, but time_stamp is just additional data to verify that the ref is distant.
The splay tree implicitly supports an array, where the element index is the number of node that are to the left. I took this idea from an implicit treap. I also benchmarked implementation based on implicit treap, but it was slowed by 5-15%.

The splay tree maintains this invariant, but time_stamp is just additional data to verify that the ref is distant.

Add to comment?

Returned the skip list implementation, implemented base classes for containers and refs, and added the ability to select an implementation using options.

artur-abd · 2026-03-26T14:51:04Z

@abhinav92003, thanks for your comments, I added an implementation selection using a flag, and also fixed the tests to test both implementations.

abhinav92003 · 2026-04-01T14:27:11Z

@abhinav92003, thanks for your comments, I added an implementation selection using a flag, and also fixed the tests to test both implementations.

Thanks; will review the new changes.

abhinav92003

I have just a few minor comments. LGTM.

Thanks for the contribution. This is a useful optimization. And apologies for the (much) longer than usual turnaround time in my reviews.

abhinav92003 · 2026-04-09T13:55:13Z

+    DROPTION_SCOPE_FRONTEND, "reuse_splay_tree", false,
+    "Use splay tree for reuse distance calculation.",
+    "Uses a splay tree data structure instead of a skip list for tracking reuse "
+    "distances. Splay tree provide better complexity and consume less memory "


s/better complexity/better time complexity/

abhinav92003 · 2026-04-09T14:11:01Z

 };

+struct line_ref_entry_t {
+    uint64_t time_stamp;   // the most recent reference time stamp on this line


Either delete the default constructor if we don't need it, or add in-place init for these members. Same for other structs.

abhinav92003 · 2026-04-09T14:16:34Z

    }
 };

+/* A splay tree node for the cache line reference info */


Add banner comment (example at https://dynamorio.org/page_code_style.html) to separate out the splay tree related functions.

abhinav92003 · 2026-04-09T14:24:46Z

+    {
+        std::cerr << "Reuse tag list:\n";
+        line_ref_node_t *node = root_;
+        // the last visited node


I suppose this is the last visited node in the current node's subtree? Or if there's none such, it's nullptr. Update comment.

abhinav92003 · 2026-04-09T14:45:33Z

 // more efficient computation of the depth.  Each node in the skip
 // list stores its depth from the front.
-struct line_ref_list_t {
+struct line_ref_list_t : public line_ref_container_t {


As per https://dynamorio.org/page_code_style.html, this should be a class as well because it has methods other than the constructor.

abhinav92003 · 2026-04-09T14:47:34Z

+// The splay tree is a binary search tree that uses the splay operation for balancing,
+// which allows deleting and inserting an element at any position in O(log n) amortized
+// time.
+class line_ref_splay_t : public line_ref_container_t {


It will be nicer if the implementation could be moved to the cpp file, instead of the header.

abhinav92003 · 2026-04-09T14:53:05Z

+            new_parent->parent = node->parent;
+        }
+        if (node->parent == nullptr)
+            root_ = new_parent;


Okay, since splay has the preconditions to call it. Add comment by the assert at L873?

abhinav92003 · 2026-04-09T14:53:16Z

+        }
+
+        if (node->parent == nullptr)
+            root_ = new_parent;


Okay, since splay has the preconditions to call it. Add comment by the assert at L848?

abhinav92003 · 2026-04-09T14:55:30Z

            return 0;
+        splay(ref);
+        // Get the reuse distance of ref.
+        int64_t dist = get_size(ref->left);


Adding this to the code comment would be helpful to a future reader.

abhinav92003 · 2026-04-09T14:56:09Z

+    struct line_ref_node_t *right;  // the right child of the node
+    struct line_ref_node_t *parent; // the parent of the node
+    uint64_t size;                  // Size of the subtree
+    uint64_t time_stamp;            // the most recent reference time stamp on this line


The splay tree maintains this invariant, but time_stamp is just additional data to verify that the ref is distant.

Add to comment?

artur-abd changed the title ~~i#7731: Replace linked list with splay tree~~ i#7731: Optimize reuse distance collection using splay tree Nov 26, 2025

artur-abd marked this pull request as draft November 27, 2025 05:35

artur-abd force-pushed the i7731-optimize-reuse-distance branch 2 times, most recently from 4b6bffb to 3961c95 Compare November 27, 2025 13:24

artur-abd force-pushed the i7731-optimize-reuse-distance branch from 3961c95 to 1b8aa74 Compare November 27, 2025 13:26

artur-abd marked this pull request as ready for review November 27, 2025 13:37

i#7731: fix descriptions

403bdf2

Fixes description for line_ref_splay_t and it's methods Fixes: DynamoRIO#7731

abhinav92003 assigned abhinav92003 and unassigned abhinav92003 Dec 2, 2025

abhinav92003 self-requested a review December 2, 2025 19:47

abhinav92003 reviewed Dec 5, 2025

View reviewed changes

artur-abd added 7 commits January 23, 2026 15:20

Merge 'master' into i7731-optimize-reuse-distance

721fbdd

i#7731: Deprecate options for skip list

7f21383

i#7731: Removed deprecated arguments

eccaeaf

i#7731: Added tests for splay tree

dcafa9f

i#7731: Fixed codestyle

b62f4cf

i#7731: Fixed a test for splay tree

bffec90

i#7731: Fixed typo

1ebbd03

Copilot AI review requested due to automatic review settings January 23, 2026 12:30

Copilot started reviewing on behalf of artur-abd January 23, 2026 12:30 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

artur-abd added 6 commits January 23, 2026 16:01

i#7731: Fixed typo

7d1dd87

i#7731: Removed unused include

a5e2a8e

i#7731: Fixed droption output & marked skip_verify as deprecated

940a0d6

i#7731: Fixed codestyle

0924683

i#7731: Separated tests for splay tree

7af338a

i#7731: Fixed codestyle

0504768

artur-abd added 2 commits January 23, 2026 19:31

i#7731: Fixed double free of nodes in tests

047689c

i#7731: Fixed codestyle

afda315

artur-abd requested a review from abhinav92003 January 23, 2026 17:01

abhinav92003 requested changes Mar 24, 2026

View reviewed changes

artur-abd added 18 commits March 25, 2026 18:35

i#7731: Make splay_tree_t as class

95a6936

i#7731: Removed redundant logic from remove

bcf5fb1

i#7731: Fixed traversal of splay tree

dc699ee

i#7731: Added assert to verify that tail_ don't have right child

0fa048b

i#7731: Added assert to verify that tail_ is right child of parent

6510f7e

i#7731: Added assert to verify that head_ has no left child

990a292

i#7731: Added asserts in rotate to check new_parent is not nullptr

98d6a07

i#7731: Added is_left_child and is_right_child

a8c8888

i#7731: Added check left_tail has no right child

8059400

i#7731: Fixed order of arguments

fcae877

i#7731: Fixed declaration of line_ref_splay_t

b60d252

i#7731: Fixed nullptr dereference

85cb1e2

i#7731: Fixed using of get_size

5a86de2

i#7731: Added comment for splay in move_to_front

f319038

i#7731: Added option for splay tree

88ed25f

i#7731: Returned the skip list implementation

9129e04

Returned the skip list implementation, implemented base classes for containers and refs, and added the ability to select an implementation using options.

i#7731: Fixed using of line_ref_node_t instead of build_entry

3064c42

i#7731: Added tests for both implementations

d12af1a

artur-abd requested a review from abhinav92003 March 26, 2026 14:51

i#7731: Fixed option name & typo

0589d25

abhinav92003 approved these changes Apr 9, 2026

View reviewed changes

Conversation

artur-abd commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekbruening commented Nov 28, 2025

Uh oh!

artur-abd commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artur-abd commented Jan 23, 2026

Uh oh!

artur-abd commented Mar 20, 2026

Uh oh!

abhinav92003 commented Mar 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artur-abd commented Mar 26, 2026

Uh oh!

abhinav92003 commented Apr 1, 2026

Uh oh!

abhinav92003 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artur-abd commented Nov 26, 2025 •

edited

Loading

artur-abd commented Dec 1, 2025 •

edited

Loading