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

Factories rework #663

Closed
wants to merge 1 commit into from
Closed

Factories rework #663

wants to merge 1 commit into from

Conversation

nolan-veed
Copy link
Contributor

Why

For #610

This is for the factories rework.

What

  • Made Context hold pools, not factories.

Testing

WIP

@github-actions github-actions bot added the work in progress Pull request is still in progress and changing label Dec 27, 2023
@github-actions GitHub Actions
Copy link

🤖 Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats.

nolan-veed
nolan-veed commented Dec 27, 2023
View reviewed changes
src/internal_modules/roc_node/context.cpp
, encoding_map_(arena_)
// TODO:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gavv It seems the network_loop_ below requires the factories. Do I pass the pools into it and let it create it's own factories?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, sounds good. roc_node will instantiate arenas pools and pass them to roc_netio and roc_pipeline. They, in turn, will instantiate factories when needed.

@gavv gavv added the contribution A pull-request by someone else except maintainers label Dec 28, 2023
@roc-streaming roc-streaming deleted a comment from github-actions bot Dec 29, 2023
@gavv
Copy link
Member

gavv commented May 14, 2024

Hi!

I'm currently working on PLC API (#305), and I need to implement #697 for it, which partially overlaps with this PR.

If you don't mind, I'm going to cherry pick & finish changes made here to my branch, and after it's merged you can proceed with this task when/if you wish.

@github-actions github-actions bot added the needs rebase Pull request has conflicts and should be rebased label May 15, 2024
@github-actions GitHub Actions
Copy link

🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts.

@gavv
Copy link
Member

gavv commented May 15, 2024

Done, see these 3 commits:

(especially the last one)

I think this part of the task is done now:

factories

  • PacketFactory, BufferFactory: accept IPool instead of IArena
  • node::Context: instantiate pools instead of factories
  • roc_pipeline: pass pools instead of factories
  • roc_pipeline: SenderSession, ReceiverSession, etc: instantiate factories from pools where they're needed

Note that BufferFactory was replaced with FrameFactory, see commit messages for details.

In roc_pipeline, factories are currently created in ReceiverSource and SenderSink and passed to lower-level components like ReceiverSession and SenderSession. For implementing per-session limits, we may need to pass pools instead, but that would be an easy change isolated in roc_pipeline.

@gavv
Copy link
Member

gavv commented May 15, 2024

I think we can close this PR? Did I miss anything?

@gavv
Copy link
Member

gavv commented May 30, 2024

Closing, feel free to ping me if I missed anything.

@gavv gavv closed this May 30, 2024
@gavv gavv added merged manually and removed work in progress Pull request is still in progress and changing needs rebase Pull request has conflicts and should be rebased labels May 30, 2024
@nolan-veed
Copy link
Contributor Author

Closing, feel free to ping me if I missed anything.

No prob. Thanks @gavv for bearing with me.

@gavv
Copy link
Member

gavv commented May 30, 2024

No worries!

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

@gavv gavv gavv left review comments

Assignees
No one assigned
Labels
contribution A pull-request by someone else except maintainers merged manually
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nolan-veed @gavv