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

Reduce unnecessary string creation when interacting with rocksdb #1583

Open
wu-hanqing opened this issue Jun 16, 2022 · 11 comments
Open

Reduce unnecessary string creation when interacting with rocksdb #1583

wu-hanqing opened this issue Jun 16, 2022 · 11 comments
Labels
Developer Activities enhancement improve feature good first issue Good for newcomers repairing bug repairing

Comments

@wu-hanqing
Copy link
Contributor

wu-hanqing commented Jun 16, 2022

Now, we use rocksdb as metadata storage, for Get and Iterate operations, rocksdb yields rocksdb::Slice that stands for key/values.

For rocksdb::Slice, is almost same with string_view in C++17, StringPiece in chromium. And they have the same idea behind it - they are just views of strings, and don't own the underlying string. And they may give some improvement in some cases.

However, in some source code, we don't follow this design, for example, RocksDBStorageIterator::Value returns a std::string which is creating from a rocksdb::Slice and does memcpy in this progress. But, in most cases, we just parse a protobuf Message from the string which means the creation is unnecessary.

@wu-hanqing wu-hanqing added the enhancement improve feature label Jun 16, 2022
@cw123 cw123 added the good first issue Good for newcomers label Jul 4, 2022
@wu-hanqing wu-hanqing changed the title Reduce unnecessary string creation when interact with rocksdb Reduce unnecessary string creation when interacting with rocksdb Jul 4, 2022
@zhanghuidinah
Copy link
Member

/assign @ccmaxcc

@zhanghuidinah
Copy link
Member

@ccmaxcc Are you having some trouble? If so, please let us know so that we can communicate together.

@wuhongsong
Copy link
Contributor

@ccmaxcc

@liam-thunder
Copy link

Interested in CurveFS, is this PR still available to contribute? I can start with this PR to get familiar with CurveFS.

@ilixiaocui
Copy link
Contributor

Interested in CurveFS, is this PR still available to contribute? I can start with this PR to get familiar with CurveFS.

@liam-thunder You are very welcome to contribute to this PR.

@liam-thunder
Copy link

address this issue in #2183

@wuhongsong wuhongsong added the repairing bug repairing label Feb 15, 2023
@Cyber-SiKu
Copy link
Contributor

@liam-thunder Is this still going on?

@liam-thunder
Copy link

@liam-thunder Is this still going on?

Sorry for late response, this issue is blocked by E2E benchmark testing and performance analysis, and requiring some extra efforts. But I'm quite busy recently, will get back on this issue later.

@Cyber-SiKu
Copy link
Contributor

@liam-thunder Are you still going on?

@liam-thunder
Copy link

@liam-thunder Are you still going on?

Yes, I'm working on deploy a single node cluster to do the benchmark.

@ilixiaocui
Copy link
Contributor

@liam-thunder Is this still going on?

Sorry for late response, this issue is blocked by E2E benchmark testing and performance analysis, and requiring some extra efforts. But I'm quite busy recently, will get back on this issue later.

If you have any questions during the benchmarking process, please feel free to ask. You can also communicate in the WeChat group.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Activities enhancement improve feature good first issue Good for newcomers repairing bug repairing
Projects
None yet
Development

No branches or pull requests

7 participants