Skip to content

fix: cache the NodeAddressString#231

Open
twoeths wants to merge 1 commit intomasterfrom
tuyen/cache-NodeAddressString
Open

fix: cache the NodeAddressString#231
twoeths wants to merge 1 commit intomasterfrom
tuyen/cache-NodeAddressString

Conversation

@twoeths
Copy link
Contributor

@twoeths twoeths commented Jan 4, 2023 •
edited
Loading

Motivation

  • Even through the cost of computing NodeAddressString may not be a lot, it's redundant to keep computing it in the same flow
  • An example of this is the "handleFindNodes" flow where we compute NodeAddressString 5 times:
    • Session service to compute NodeAddressString, then send to Discv5 service
    • Discv5 service find 16 nodes, with 4 node per packet it'd send 4 responses
    • For each response we compute NodeAddressString again

Description

  • Cache NodeAddressString inside the INodeAddress
  • In the above flow it'd create 1 instead of 5 string

part of #201

@twoeths twoeths requested a review from a team as a code owner January 4, 2023 03:53
@wemeetagain wemeetagain force-pushed the master branch 2 times, most recently from b3e0a17 to 938e0fb Compare November 29, 2023 15:59
@philknows philknows mentioned this pull request Feb 28, 2024
Closed
86 tasks
@wemeetagain
Copy link
Member

wemeetagain commented Oct 8, 2024

Can you show the benefit in terms of CPU and/or memory savings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@dapplion dapplion dapplion approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@twoeths @wemeetagain @dapplion