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

feat(ShardAccessor): feat: enable concurrent ShardAccessor usage with mmap #2

Merged
merged 5 commits into from
Apr 4, 2023

Conversation

distractedm1nd
Copy link
Member

@distractedm1nd distractedm1nd commented Apr 1, 2023

Related: celestiaorg/celestia-node#1514. Will open a PR there that adds a test to verify this functionality in the LRU cache, and enables cache usage for GetCAR and GetDAH. This will allow shrexeds and shrexnd servers to only require opening the file once (until removed from cache)

  • Update the Read method to use mmapr when available, falling back to the data reader if not
  • Refactor the Blockstore method to reuse the mmapr field if it is already set, preventing unnecessary file opens

This change enables concurrent access to the ShardAccessor without opening the file multiple times, improving performance and resource usage.

… mmap

- Modify the ShardAccessor struct to use the existing mmapr field for concurrent access
- Update the Read method to use mmapr when available, falling back to the data reader if not
- Refactor the Blockstore method to reuse the mmapr field if it is already set, preventing unnecessary file opens
- This change enables concurrent access to the ShardAccessor without opening the file multiple times, improving performance and resource usage.
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Ok with me - can we get Raul to take a look as well or maybe someone at PL so they can let us know if this is upstream-able?

accessor.go Outdated Show resolved Hide resolved
accessor.go Outdated Show resolved Hide resolved
accessor.go Show resolved Hide resolved
accessor.go Outdated Show resolved Hide resolved
accessor.go Outdated Show resolved Hide resolved
accessor.go Outdated Show resolved Hide resolved
accessor.go Outdated Show resolved Hide resolved
distractedm1nd and others added 2 commits April 4, 2023 14:27
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

So compact now, lovely

@distractedm1nd distractedm1nd merged commit 177451f into master Apr 4, 2023
distractedm1nd added a commit to celestiaorg/celestia-node that referenced this pull request Apr 4, 2023
Closes #1514 . This PR enables accessor cache usage for GetCAR and
GetDAH. This will allow shrexeds and shrexnd servers to only require
opening the underlying file once (until removed from cache). If many
peers request the EDS at once, the server previously needed to open the
file each time.

Will still require a dependency bump of our dagstore fork for the new
test to not fail - so draft until then. Related:
celestiaorg/dagstore#2

---------

Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
walldiss pushed a commit that referenced this pull request Jul 24, 2023
… mmap (#2)

* feat(ShardAccessor): feat: enable concurrent ShardAccessor usage with mmap

- Modify the ShardAccessor struct to use the existing mmapr field for concurrent access
- add `io.Reader` factory, `Reader() io.Reader` produces readers that use mmapr when available, falling back to the data reader if not
- Refactor the Blockstore method to reuse the mmapr field if it is already set, preventing unnecessary file opens
- This change enables concurrent access to the ShardAccessor without opening the file multiple times, improving performance and resource usage.

* remove comment

* refactor: `io.Reader` factory, instead of implementing iface directly

* Update accessor.go

Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>

* wrapping error

---------

Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
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.

3 participants