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

Optimize the Client Deactivate flow to enhance performance #1039

Open
raararaara opened this issue Oct 18, 2024 · 1 comment
Open

Optimize the Client Deactivate flow to enhance performance #1039

raararaara opened this issue Oct 18, 2024 · 1 comment
Labels
enhancement 🌟 New feature or request

Comments

@raararaara
Copy link
Contributor

raararaara commented Oct 18, 2024

What would you like to be added:
The recent update regarding the Deactivate flow of the Client in PR #1036 has highlighted several performance-related issues, documented in the provided DB query and Client connection flow.

Here are the key areas identified for improvement:

  1. Remove duplicate DB resource requests:
    There are overlapping DB queries between clients.Deactivate and server.DetachDocument, specifically FindActiveClientInfo and FindDocInfoByRefKey. We need to eliminate these duplicate requests to enhance efficiency.

  2. Optimize DocInfo query within clients.Deactivate:
    When multiple documents are attached to a Client, we currently face an N+1 query problem. We should implement a way to fetch the list of documents attached to the Client in a single query to avoid performance degradation.

    for docID, clientDocInfo := range info.Documents {
    if clientDocInfo.Status != database.DocumentAttached {
    continue
    }
    // TODO(hackerwins): Solve N+1
    docInfo, err := be.DB.FindDocInfoByRefKey(ctx, types.DocRefKey{
    ProjectID: project.ID,
    DocID: docID,
    })

  3. Improve ChangePack creation logic for p.Clear() operation:
    In the current implementation, the server temporarily loads documents to create a ChangePack for presence clear operations. Given that creating documents from a given checkpoint through FindChangesBetweenServerSeqs is resource-intensive, we should explore a method to create the ChangePack more directly, reducing unnecessary DB resource requests.

    // TODO(hackerwins): BuildDocForCheckpoint is expensive because it reads the entire document.
    // We need to optimize this by creating a ChangePack directly.
    // 01. Create ChangePack with clear presence.
    doc, err := packs.BuildDocForCheckpoint(ctx, s.backend, docInfo, clientInfo.Checkpoint(summary.ID), actorID)
    if err != nil {
    return nil, err
    }
    if err := doc.Update(func(root *json.Object, p *presence.Presence) error {
    p.Clear()
    return nil
    }); err != nil {
    return nil, err
    }

  4. Refactor clusterClient Connection management:
    Currently, a new Client connection is established with each Deactivate call, which incurs substantial overhead. We need to consider ways to minimize this cost and implement appropriate timeout settings related to the connections.

    // TODO(hackerwins): Introduce cluster client pool.
    // - https://connectrpc.com/docs/go/deployment/
    cli, err := cluster.Dial(be.Config.GatewayAddr)
    if err != nil {
    return nil, err
    }

Why is this needed:
Enhancing the Deactivate flow will improve overall performance, reduce database load, and streamline the Client's lifecycle management, ultimately leading to a more efficient and responsive application.

@raararaara
Copy link
Contributor Author

I've measured the execution time for each step in the client deactivation process.

This test was run on a client with a single attached document, and while results may vary based on document size, it provides a relative comparison across the steps.

Running locally, we observed the following execution times for a case with a total duration of 150ms:

  • FindActiveClientInfo, FindDocInfoByRefKey: ~3ms
  • BuildDocForCheckpoint: 100ms
  • clusterClient Connection: ~1ms

It’s clear that BuildDocForCheckpoint incurs a relatively higher processing cost compared to the other steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🌟 New feature or request
Projects
Status: Backlog
Development

No branches or pull requests

1 participant