Skip to content

Implemented QuART_tail, QuART_xtail, QuART_lil and refactored codebase - #21

Closed
cangokmen wants to merge 93 commits into
mainfrom
cangokmen/new_tail_method
Closed

Implemented QuART_tail, QuART_xtail, QuART_lil and refactored codebase#21
cangokmen wants to merge 93 commits into
mainfrom
cangokmen/new_tail_method

Conversation

@cangokmen

Copy link
Copy Markdown
Contributor

Changed files: ART.h, ArtNode.h, .gitignore, CMakeLists.txt, README.md
Created files: ArtNodeNewMethods.cpp, QuARTVariants/QuART_tail.h, QuARTVariants/QuART_xtail.h, QuARTVariants/QuART_lil.h, benchmarks/profile_inserts.cpp, benchmarks/art.cpp, benchmarks/quart_tail.cpp, benchmarks/quart_xtail.cpp, benchmarks/quart_lil.cpp, run_experiments.sh

  • ART.h: Modified insert function names to make them distinguishable when profiling. Added printTree method that pretty-prints the tree. Added verifyTailPath method that returns whether the tail path is true (can be used in QuART_tail and QuART_xtail trees for verification).
  • ArtNode.h: Added declarations of insertNodeX methods used in fast path insertion. Added printFpPath method that pretty-prints a node array.
  • ArtNodeNewMethods.cpp: Implemented insertNodeX functions used in fast path insertion. Follow the comments to see which part does what.
  • QuARTVariants/QuART_tail.h: Implemented the generic tail insert.
  • QuARTVariants/QuART_xtail.h: Implemented a fp method similar to tail insert but faster (name recommendations are greatly appreciated).
  • QuARTVariants/QuART_lil.h: Implemented the generic lil insert.
  • benchmarks/profile_inserts.cpp: Fragment with only tree inserts, use this during profiling. It doesn't read input file, only inserts integers in sorted order from 1 to 10000000. To profile, run perf record ./profile_inserts -v -N - 10000000 (N < 10000000), then perf report.
  • benchmarks/art.cpp: Created executable that will run the base ART algorithm.
  • benchmarks/quart_tail.cpp: Created executable that will run the tail insert algorithm.
  • benchmarks/quart_xtail.cpp: Created executable that will run the xtail insert algorithm.
  • benchmarks/quart_lil.cpp: Created executable that will run the lil insert algorithm.
  • run_experiments.sh: Runs the benchmark experiments.
  • .gitignore: Updated gitignore so that it ignores the results folder.
  • CMakeList.txt: Added necessary lines to use perf, and added the executables for benchmark files.
  • README.md: Updated README with the new changes. It explains how to run the experiments too.

Can Gokmen and others added 30 commits June 18, 2025 14:36
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

This comment was marked as outdated.

@cangokmen
cangokmen requested a review from Copilot July 9, 2025 03:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces three new QuART variants (tail, xtail, lil) with fast-path insertion strategies, refactors the core ART implementation to support path printing/verification, and adds benchmark tooling along with documentation updates.

  • Implemented QuART_tail, QuART_xtail, and QuART_lil insert methods in the QuARTVariants directory
  • Extended ART core (ART.h, ArtNode.h, ArtNodeNewMethods.cpp) to initialize and track fast-path (fp), printing, and verification
  • Added per-variant benchmark executables, an experiment script (run_experiments.sh), updated CMake targets, and refreshed README.md

Reviewed Changes

Copilot reviewed 15 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
run_experiments.sh Script to automate running all variants over workloads
benchmarks/quart_*.cpp Added benchmark drivers for xtail, tail, and lil variants
benchmarks/profile_inserts.cpp Benchmark-only insert driver; loads sequential keys
benchmarks/art.cpp Updated ART benchmark to default N=10M and removed verbose logic
CMakeLists.txt New targets for all benchmarks; adjusted compiler flags
README.md Documented new executables, script usage, and variant descriptions
QuARTVariants/*.h Core implementations for QuART_tail, QuART_xtail, QuART_lil
ART.h, ArtNode.h Initialized members for fast-path, added printTree & verifyTailPath
ArtNodeNewMethods.cpp Implemented fast-path insertNodeX methods and debugging helpers
Comments suppressed due to low confidence (3)

benchmarks/profile_inserts.cpp:39

  • [nitpick] The comment above mentions changing the tree to ART::ART, but the code still instantiates QuART_xtail. Update the comment or the code to match the intended tree type.
    ART::QuART_xtail* tree = new ART::QuART_xtail();

README.md:52

  • [nitpick] The README still documents a '-v' flag for executables, but the benchmark code (e.g., art.cpp) no longer supports verbose mode. Remove or update references to '-v'.
- `-v`: Verbose mode (optional, default = false)

run_experiments.sh:1

  • [nitpick] Add a shebang (e.g., #!/usr/bin/env bash) at the top of the script and ensure that variables like LOGDIR, RESULTS, SUFFIX, and REPEAT are defined or passed in before use.
for FILE in ../bods/workloads/workload_N*_K*_L*.bin; do

Comment thread benchmarks/quart_xtail.cpp
Comment thread benchmarks/quart_tail.cpp
Comment thread benchmarks/quart_lil.cpp
Comment thread CMakeLists.txt

@ramananeesh ramananeesh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please address the changes. Also, make sure you use .clang-format to format each of your files.

Comment thread ART.h Outdated
Comment thread ART.h Outdated
Comment thread ART.h Outdated
Comment thread ArtNode.h

#include "Helper.h"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get rid of this line. Make sure you are formatting the files using the clang-format file at the base directory in the repo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do the formatting on Thursday

Comment thread ArtNode.h Outdated
Comment thread ArtNode.h
size_t& temp_fp_path_length, size_t depth_prev);
void eraseNode256(ART* tree, ArtNode** nodeRef, uint8_t keyByte);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove empty line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do the formatting on Thursday

Comment thread ArtNode.h
this->child[keyByte] = child;
}


Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove empty line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do the formatting on Thursday

Comment thread QuARTVariants/QuART_lil.h

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have QuART_lil in this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can put it in a seperate PR, it was easy to implement after having the tail implementation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always, keep PRs atomic so that it is easy to track changes. Coupling multiple features will only make the codebase messy if we need to rollback.

Also, since this was something @NoahP-K was trying to implement, move this to a separate branch and collaborate with him on this. We can make a new PR.

Comment thread QuARTVariants/QuART_tail.h Outdated
Comment thread benchmarks/quart_lil.cpp

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use a single benchmark file that can compile different trees, instead of duplicated benchmark code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I do this in this PR or the next PR?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you. Technically, if these files are created in this PR, you can fix it here. However, if you feel that getting the rest of the features merged into main will be beneficial for the whole group, you can choose to do it in a separate PR as well.

@cangokmen
cangokmen force-pushed the cangokmen/new_tail_method branch from 58b6b20 to 73aea79 Compare July 9, 2025 17:32
@cangokmen
cangokmen force-pushed the cangokmen/new_tail_method branch from fe7ee3a to 45350c3 Compare July 11, 2025 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants