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: provide redis shared memory plugin #345

Closed
wants to merge 3 commits into from

Conversation

n063h
Copy link
Contributor

@n063h n063h commented May 6, 2023

provide RedisShmWrapper class and an example.

@github-actions github-actions bot added the enhancement New feature or request label May 6, 2023
@@ -4,7 +4,7 @@ This is an example demonstrating how you can enable the plasma shared memory sto

Mosec's multi-stage pipeline requires the output data from the previous stage to be transferred to the next stage across python processes. This is coordinated via Unix domain socket between every Python worker process from all stages and the Rust controller process.

By default, we serialize the data and directly transfer the bytes over the socket. However, users may find wrapping this IPC useful or more efficient for specific use cases. Therefore, we provide the `mosec.plugins.IPCWrapper` interface and an example implementation `PlasmaShmWrapper` based on [`pyarrow.plasma`](https://arrow.apache.org/docs/python/plasma.html).
By default, we serialize the data and directly transfer the bytes over the socket. However, users may find wrapping this IPC useful or more efficient for specific use cases. Therefore, we provide the `mosec.plugins.IPCWrapper` interface and example implementation `PlasmaShmWrapper` based on [`pyarrow.plasma`](https://arrow.apache.org/docs/python/plasma.html) and `RedisShmWrapper` based on [`redis`](https://pypi.org/project/redis). We recommand using `RedisShmWrapper` for better performance.
Copy link
Member

Choose a reason for hiding this comment

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

Is "better performance" verified experimentally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, based on the benchmark experiment at https://github.com/mosecorg/numbin/blob/master/benchmark/bench.py, the results show that when comparing the performance ,Redis connected via a Unix socket > Plasma > Redis connected via a TCP socket.

Copy link
Member

Choose a reason for hiding this comment

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

Cool!

But the benchmark file mentioned above only tests the serialization / de-serialization performance, so you modify it to test the shared memory performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's a repo of serde library, I reused the cases while did not update it.

Copy link
Member

Choose a reason for hiding this comment

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

I think the code and benchmark data can be uploaded to mosecorg/benchmark. We can add the link here.

Signed-off-by: hang lv <xlv20@fudan.edu.cn>
return str(number).encode()

def bytes_id_gen() -> bytes:
int_id_gen = partial(self.client.incr, DEFAULT_KEY)
Copy link
Member

Choose a reason for hiding this comment

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

If I run multiple mosec instances, will they share the same key to the same redis?

Copy link
Member

Choose a reason for hiding this comment

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

@n063h Could you please add something (e.g., timestamp) to differentiate?

@kemingy
Copy link
Member

kemingy commented May 19, 2023

The implementation can refer to the new plasma mixin https://github.com/mosecorg/mosec/blob/main/mosec/mixin/plasma_worker.py

@kemingy kemingy linked an issue May 19, 2023 that may be closed by this pull request
@n063h n063h closed this May 24, 2023
@n063h
Copy link
Contributor Author

n063h commented May 24, 2023

Since redis shm is now implemented as a mixin worker, refer to #367, instead of plugin, this pr is closed.

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

Successfully merging this pull request may close these issues.

feat: provide redis shared memory mixin
3 participants