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

/delegates performance optimization #162

Merged
merged 5 commits into from
Mar 12, 2024
Merged

Conversation

Flip-Liquid
Copy link
Collaborator

@Flip-Liquid Flip-Liquid commented Mar 11, 2024

  • Data retrieved as part of generateMetadata is not cached, and is re-retrieved for the Page itself -> cache this in React's cache (see Next.js recommendation)
  • Delegate statement is quadruple-retrieved: once each in
    • generateMetadata, explicitly
    • generateMetadata, as part of getDelegate
    • Page, explicitly
    • Page, as part of getDelegate
      -> Remove explicit delegate statement retrieval, cache delegate retrieved from generateMetadata
  • Every query resolves the ENS name to an address -> do this once in generateMetadata and cache for lifetime of request

Stats for full page load times, TL;DR: 35% decrease in median latency (snapshot reflects a different statistic):
image


PR-Codex overview

This PR updates caching mechanisms using react cache and optimizes ENS resolution in multiple files.

Detailed summary

  • Added cache import from react for caching
  • Refactored ENS resolution using caching for better performance
  • Improved handling of ENS names and addresses in metadata generation

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

vercel bot commented Mar 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
agora-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 12, 2024 0:15am

Copy link
Contributor

@andreitr andreitr left a comment

Choose a reason for hiding this comment

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

Solid work! I left a few minor comments. The TLDR - let's move caching upstream.

resolveENSProfileImage,
} from "@/app/lib/ENSUtils";

// TODO: -> see if react cache can be used in fetchDelegate
const getCachedDelegate = cache(
Copy link
Contributor

Choose a reason for hiding this comment

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

I will be curious to see how unstable_cache compares to react cache.

My only comment is to implement this caching on fetchDelegate inside /app/delegates/actions.ts. This way all requests to fetchDelegates are automatically cached.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, good catch, agree.

const getCachedAddress = cache(
(addressOrENSName: string) => resolveENSName(addressOrENSName)
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, let's add caching resolveENSName call itself.

Copy link
Contributor

@stepandel stepandel left a comment

Choose a reason for hiding this comment

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

fire!

considering how great of an improvement it is, we should apply this server actions caching pattern across the application

@Flip-Liquid
Copy link
Collaborator Author

Flip-Liquid commented Mar 12, 2024 via email

Copy link
Contributor

@andreitr andreitr left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@Flip-Liquid Flip-Liquid merged commit 7ff67e8 into main Mar 12, 2024
2 checks passed
@Flip-Liquid Flip-Liquid deleted the flip/delegate_caching branch March 12, 2024 16:01
@ferrodri
Copy link
Collaborator

ferrodri commented Mar 12, 2024

  • Data retrieved as part of generateMetadata is not cached, and is re-retrieved for the Page itself -> cache this in React's cache (see Next.js recommendation)

  • Delegate statement is quadruple-retrieved: once each in

    • generateMetadata, explicitly
    • generateMetadata, as part of getDelegate
    • Page, explicitly
    • Page, as part of getDelegate
      -> Remove explicit delegate statement retrieval, cache delegate retrieved from generateMetadata
  • Every query resolves the ENS name to an address -> do this once in generateMetadata and cache for lifetime of request

Stats for full page load times, TL;DR: 35% decrease in median latency (snapshot reflects a different statistic): image

PR-Codex overview

This PR updates caching mechanisms using react cache and optimizes ENS resolution in multiple files.

Detailed summary

  • Added cache import from react for caching
  • Refactored ENS resolution using caching for better performance
  • Improved handling of ENS names and addresses in metadata generation

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@Flip-Liquid could you share with me the python script to use to get the stats and load times? It would help me with QA, thank you!

Copy link

pr-codex bot commented Mar 12, 2024

{your question}`

@Flip-Liquid could you share the python script to use to get the stats and load times? It would really help me with QA, thank you!

@ferrodri I can't provide a Python script for that purpose as the code diff provided is related to TypeScript and JavaScript files, not Python. If you have any questions related to the TypeScript or JavaScript code diff, feel free to ask!

@Flip-Liquid
Copy link
Collaborator Author

  • Data retrieved as part of generateMetadata is not cached, and is re-retrieved for the Page itself -> cache this in React's cache (see Next.js recommendation)

  • Delegate statement is quadruple-retrieved: once each in

    • generateMetadata, explicitly
    • generateMetadata, as part of getDelegate
    • Page, explicitly
    • Page, as part of getDelegate
      -> Remove explicit delegate statement retrieval, cache delegate retrieved from generateMetadata
  • Every query resolves the ENS name to an address -> do this once in generateMetadata and cache for lifetime of request

Stats for full page load times, TL;DR: 35% decrease in median latency (snapshot reflects a different statistic): image

PR-Codex overview

This PR updates caching mechanisms using react cache and optimizes ENS resolution in multiple files.

Detailed summary

  • Added cache import from react for caching
  • Refactored ENS resolution using caching for better performance
  • Improved handling of ENS names and addresses in metadata generation

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@Flip-Liquid could you share with me the python script to use to get the stats and load times? It would help me with QA, thank you!

@ferrodri here ya go:

# Invoke as: python3 test_localhost.py > output.txt
# requires delegates.txt as input in same directory 
import requests
import time

# Replace 'your_file.txt' with the path to your file
file_path = 'delegates.txt'

# Function to make a GET request
def make_get_request(value):
    url = f'http://localhost:3000/delegates/{value}'
    start_time = time.time()  # Start timing

    response = requests.get(url)

    end_time = time.time()  # Stop timing
    duration = end_time - start_time

    print(f'Value: {value}, Status Code: {response.status_code}, Duration: {duration:.2f}s')

# Iterate over each line in the file
with open(file_path, 'r') as file:
    for line in file:
        value = line.strip()  # Remove any leading/trailing whitespace
        make_get_request(value)

not very modular, but we could maybe fix that
^ should we check this in?

Copy link

pr-codex bot commented Mar 12, 2024

{your question}`

@Flip-Liquid could you share with me the python script to use to get the stats and load times? It would help me with QA, thank you!

@ferrodri here ya go:

# Invoke as: python3 test_localhost.py > output.txt
# requires delegates.txt as input in same directory 
import requests
import time

# Replace 'your_file.txt' with the path to your file
file_path = 'delegates.txt'

# Function to make a GET request
def make_get_request(value):
    url = f'http://localhost:3000/delegates/{value}'
    start_time = time.time()  # Start timing

    response = requests.get(url)

    end_time = time.time()  # Stop timing
    duration = end_time - start_time

    print(f'Value: {value}, Status Code: {response.status_code}, Duration: {duration:.2f}s')

# Iterate over each line in the file
with open(file_path, 'r') as file:
    for line in file:
        value = line.strip()  # Remove any leading/trailing whitespace
        make_get_request(value)

not very modular, but we could maybe fix that
^ should we check this in?

@Flip-Liquid I can help with understanding the code diff you provided. Feel free to ask any specific questions you have about the changes in the code.

@ferrodri
Copy link
Collaborator

  • Data retrieved as part of generateMetadata is not cached, and is re-retrieved for the Page itself -> cache this in React's cache (see Next.js recommendation)

  • Delegate statement is quadruple-retrieved: once each in

    • generateMetadata, explicitly
    • generateMetadata, as part of getDelegate
    • Page, explicitly
    • Page, as part of getDelegate
      -> Remove explicit delegate statement retrieval, cache delegate retrieved from generateMetadata
  • Every query resolves the ENS name to an address -> do this once in generateMetadata and cache for lifetime of request

Stats for full page load times, TL;DR: 35% decrease in median latency (snapshot reflects a different statistic): image

PR-Codex overview

This PR updates caching mechanisms using react cache and optimizes ENS resolution in multiple files.

Detailed summary

  • Added cache import from react for caching
  • Refactored ENS resolution using caching for better performance
  • Improved handling of ENS names and addresses in metadata generation

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@Flip-Liquid could you share with me the python script to use to get the stats and load times? It would help me with QA, thank you!

@ferrodri here ya go:

# Invoke as: python3 test_localhost.py > output.txt
# requires delegates.txt as input in same directory 
import requests
import time

# Replace 'your_file.txt' with the path to your file
file_path = 'delegates.txt'

# Function to make a GET request
def make_get_request(value):
    url = f'http://localhost:3000/delegates/{value}'
    start_time = time.time()  # Start timing

    response = requests.get(url)

    end_time = time.time()  # Stop timing
    duration = end_time - start_time

    print(f'Value: {value}, Status Code: {response.status_code}, Duration: {duration:.2f}s')

# Iterate over each line in the file
with open(file_path, 'r') as file:
    for line in file:
        value = line.strip()  # Remove any leading/trailing whitespace
        make_get_request(value)

not very modular, but we could maybe fix that ^ should we check this in?

Thank you! I couldn't understand this part, what do you mean by "check this in?"

Copy link

pr-codex bot commented Mar 13, 2024

{your question}`

@Flip-Liquid could you share with me the python script to use to get the stats and load times? It would help me with QA, thank you!

@ferrodri here ya go:

# Invoke as: python3 test_localhost.py > output.txt
# requires delegates.txt as input in same directory 
import requests
import time

# Replace 'your_file.txt' with the path to your file
file_path = 'delegates.txt'

# Function to make a GET request
def make_get_request(value):
    url = f'http://localhost:3000/delegates/{value}'
    start_time = time.time()  # Start timing

    response = requests.get(url)

    end_time = time.time()  # Stop timing
    duration = end_time - start_time

    print(f'Value: {value}, Status Code: {response.status_code}, Duration: {duration:.2f}s')

# Iterate over each line in the file
with open(file_path, 'r') as file:
    for line in file:
        value = line.strip()  # Remove any leading/trailing whitespace
        make_get_request(value)

not very modular, but we could maybe fix that ^ should we check this in?

Thank you! I couldn't understand this part, what do you mean by "check this in?"

@ferrodri "Check this in" refers to the action of committing the code changes to a version control system like Git. It's a common term used to indicate that the changes are ready to be added to the codebase and tracked by the version control system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants