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.

Sign up for GitHub

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

Jump to bottom

Zero-copy reads in Redwood via Arena-based pages and ability to allocate 4k-aligned memory in Arenas #4780

Merged
merged 16 commits into from
May 18, 2021
Merged

Zero-copy reads in Redwood via Arena-based pages and ability to allocate 4k-aligned memory in Arenas #4780

merged 16 commits into from
May 18, 2021

Conversation

sfc-gh-satherton
Copy link
Collaborator

@sfc-gh-satherton sfc-gh-satherton commented May 6, 2021 •
edited
Loading

  1. Arena now has a method for allocating a 4k-aligned buffer.
  2. Redwood now uses a single non-abstract Page class called ArenaPage, which uses an Arena for its memory. This replaces 'IPage' and its two implementations.
  3. This enables Redwood to return Value and KeyValue results which do not require copying KV data for initialization, as instead their arenas can simply depend on the underlying Redwood ArenaPage and Mirror arenas.
  4. Another Pager use-after-shutdown bug was fixed as well, including adding kill(Error) to FlowLock to cause all current and future waiters on the lock to get an error.

Passes 200k correctness
20210516-100240-satherton_foundationdb.two-86be77f3b26642e2
20210516-100518-satherton_foundationdb.two-a4bc6ae516a20eaa

Code-Reviewer Section

The general guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

  • The PR has a description, explaining both the problem and the solution.
  • The description mentions which forms of testing were done and the testing seems reasonable.
  • Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

  • This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or master if this is the youngest branch)
  • There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

@sfc-gh-satherton
Replaced IPage with ArenaPage, a new Arena-based page class which ena…
06e8caa 
…bles StringRefs to hold a reference to Redwood page memory.
@sfc-gh-satherton
Added Arena::allocateAlignedBuffer() to get an aligned memory block o…
f8a8bf3 
…wned by an Arena, and ArenaPage uses this.
@sfc-gh-satherton
BTreeCursor now exposes the current Page and Mirror arenas so that us…
799e7cc 
…ers extracting Standalone Keys and Values can just reference those instead of copying.
@sfc-gh-satherton sfc-gh-satherton marked this pull request as draft May 6, 2021 04:32
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 799e7cc
  • Result: FAILED
  • Build Logs (available for 7 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: e0e3cd3
  • Result: FAILED
  • Build Logs (available for 7 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 16f9f6e
  • Result: FAILED
  • Build Logs (available for 7 days)

sfc-gh-anoyes
sfc-gh-anoyes reviewed May 6, 2021
View reviewed changes
flow/Arena.h Outdated Show resolved Hide resolved
@sfc-gh-satherton
Use of aligned_alloc() for 4k pages causes too much wasted virtual me…
2298567 
…mory. Added new 4k-aligned fast allocator, and changed Arena::allocatedAlignedBuffer() to be 4k-specific, now called Arena::allocate4kAlignedBuffer().
@sfc-gh-satherton
Bug fix: It is possible for the pager to be shut down while a cursor …
6a5bf12 
…operation is acquiring its mutex, specifically after the permit is available but before the delay(0) inside take() is ready, causing the cursor to operate on an invalid pager.
@sfc-gh-satherton
Clarity improvement, loop was reusing variable name from enclosing sc…
cfeff9a 
…ope.
@sfc-gh-satherton
Bug fix, returned value wasn't pointing into the correct arena.
b4e766b
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: b4e766b
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: cfeff9a
  • Result: FAILED
  • Build Logs (available for 7 days)

@sfc-gh-satherton
Avoid callers of getValue() and getValuePrefix() from being able to a…
bd0c4a4 
…dd arena dependencies to the source page arena.
@sfc-gh-satherton sfc-gh-satherton changed the title Arena::allocateAlignedBuffer, ArenaPage, and zero-copy reads into Standalones with Redwood Zero-copy reads in Redwood via Arena-based pages and ability to allocated 4k-aligned memory in Arenas May 16, 2021
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: bd0c4a4
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@sfc-gh-satherton sfc-gh-satherton marked this pull request as ready for review May 16, 2021 11:01
@sfc-gh-satherton sfc-gh-satherton marked this pull request as draft May 16, 2021 11:02
@sfc-gh-satherton sfc-gh-satherton marked this pull request as ready for review May 16, 2021 11:14
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: a31e4f6
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@sfc-gh-satherton sfc-gh-satherton marked this pull request as draft May 16, 2021 11:25
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: a31e4f6
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@sfc-gh-satherton sfc-gh-satherton marked this pull request as ready for review May 16, 2021 12:21
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 390b026
  • Result: FAILED
  • Build Logs (available for 7 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 829bd28
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: f88596b
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@sfc-gh-satherton sfc-gh-satherton changed the title Zero-copy reads in Redwood via Arena-based pages and ability to allocated 4k-aligned memory in Arenas Zero-copy reads in Redwood via Arena-based pages and ability to allocate 4k-aligned memory in Arenas May 17, 2021
sfc-gh-jslocum
sfc-gh-jslocum reviewed May 17, 2021
View reviewed changes
Copy link
Collaborator

@sfc-gh-jslocum sfc-gh-jslocum left a comment

Choose a reason for hiding this comment

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

LGTM. Went over all of the arena logic and it looks sound.
I'd still defer to Andrew though as he has more experience here

fdbserver/VersionedBTree.actor.cpp Outdated Show resolved Hide resolved
fdbserver/VersionedBTree.actor.cpp Outdated Show resolved Hide resolved
flow/FastAlloc.h Show resolved Hide resolved
sfc-gh-anoyes
sfc-gh-anoyes reviewed May 17, 2021
View reviewed changes
Copy link
Collaborator

@sfc-gh-anoyes sfc-gh-anoyes left a comment

Choose a reason for hiding this comment

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

I'll do a closer look too. I just noticed this for now

flow/Arena.h Outdated Show resolved Hide resolved
@sfc-gh-anoyes sfc-gh-anoyes self-requested a review May 17, 2021 20:41
sfc-gh-anoyes
sfc-gh-anoyes reviewed May 17, 2021
View reviewed changes
fdbserver/IPager.h Outdated Show resolved Hide resolved
@sfc-gh-satherton @sfc-gh-anoyes
Update fdbserver/IPager.h
e405387 
Co-authored-by: Andrew Noyes <andrew.noyes@snowflake.com>
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: e405387
  • Result: FAILED
  • Build Logs (available for 7 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 60504e1
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@sfc-gh-satherton
Implement getValuePrefix() using getValue() rather than the other way…
f30793f 
… around to avoid the common getValue()'s actor state increasing from 128 to 256 bytes since it is a very hot code path.
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 279264a
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: f30793f
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@sfc-gh-anoyes sfc-gh-anoyes self-requested a review May 18, 2021 23:30
@sfc-gh-jslocum sfc-gh-jslocum merged commit 0752fb6 into apple:master May 18, 2021
@sfc-gh-satherton sfc-gh-satherton mentioned this pull request Jul 28, 2021
Merged
5 tasks
This was referenced Feb 10, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-anoyes sfc-gh-anoyes sfc-gh-anoyes approved these changes

@sfc-gh-jslocum sfc-gh-jslocum sfc-gh-jslocum approved these changes

@sfc-gh-ngoyal sfc-gh-ngoyal Awaiting requested review from sfc-gh-ngoyal

Assignees

@sfc-gh-jslocum sfc-gh-jslocum

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sfc-gh-satherton @foundationdb-ci @sfc-gh-anoyes @sfc-gh-jslocum