Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid looking up previous version of nodes that are new #331

Merged
merged 4 commits into from
Jan 3, 2023

Conversation

afterdusk
Copy link
Member

@afterdusk afterdusk commented Jan 3, 2023

Overview

When writing tree nodes to storage, we look up previous versions of the node to create the TreeNodeWithPreviousValue struct. However, this is not necessary when we know a node is new and does not have a previous value. By keeping track of whether a node is new, we save on incurring an unnecessary round trip to the database.

This PR performs the requisite change.

I would have liked to add a test for this change, however I cannot think of a straightforward way to assert that the database is not read beyond the initial preload. I believe this is something that can be addressed by bringing in a mocking library to mock out the Database trait, or implementing an in-memory database which we can mock. Filed an issue regarding this: #332

Manual Test

Added the following test to append_only_zks.rs, which batch inserts 10 nodes on an empty tree and prints database access metrics:

#[tokio::test]
async fn test_get_count() -> Result<(), AkdError> {
    let num_nodes = 10;
    let node_set = gen_nodes(num_nodes);

    let database = AsyncInMemoryDatabase::new();
    let db = StorageManager::new(database.clone(), None, None, None);
    let mut azks = Azks::new(&db).await?;

    db.begin_transaction();
    azks.batch_insert_nodes(&db, node_set, InsertMode::Directory)
        .await?;
    db.log_metrics(log::Level::Trace).await;

    Ok(())
}

The test was ran with the following command:

$ cargo test test_get_count -- --nocapture

If the change works, no gets are expected as the insert was performed on an empty tree with nothing to preload.

Before

On the main branch, with 7878bcb cherry-picked:

===================================================
============ Database operation counts ============
===================================================
    SET 1, 
    BATCH SET 0, 
    GET 19, 
    BATCH GET 0
    TOMBSTONE 0
    GET USER STATE 0
    GET USER DATA 0
    GET USER STATE VERSIONS 0

19 gets are performed.

After

On this PR's branch:

===================================================
============ Database operation counts ============
===================================================
    SET 1, 
    BATCH SET 0, 
    GET 0, 
    BATCH GET 0
    TOMBSTONE 0
    GET USER STATE 0
    GET USER DATA 0
    GET USER STATE VERSIONS 0

Indeed the number of gets are now 0.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 3, 2023
@eozturk1
Copy link
Contributor

eozturk1 commented Jan 3, 2023

Can we get the statistics on an integration test suite to see whether we are using any GET calls that hits the database? Example in #223.

Copy link
Contributor

@slawlor slawlor left a comment

Choose a reason for hiding this comment

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

We'll follow up with automated tests once we have a storage mock imported

Copy link
Contributor

@eozturk1 eozturk1 left a comment

Choose a reason for hiding this comment

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

Thanks for sharing the metrics, looks great!

@afterdusk afterdusk merged commit 3a353e7 into facebook:main Jan 3, 2023
@afterdusk afterdusk deleted the new-nodes branch January 3, 2023 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants