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

Do not use JSON for internal routing #870

Conversation

jmillan
Copy link
Member

@jmillan jmillan commented Jul 20, 2022

JSON encoding/decoding is very CPU intensive.

Use character separated string for internal routing representation.

data info from node to worker for Channel remains being JSON.
data info from node to worker for PayloadChannel is a now string to be decoded by the target:

  • At this moment only DataChannel ppid is used as data, and we are NOT sending it as JSON but as plain string.

JSON encoding/decoding is very CPU intensive.

Use character separated string for internal routing representation.
@jmillan jmillan requested a review from ibc July 20, 2022 09:17
@jmillan
Copy link
Member Author

jmillan commented Jul 20, 2022

Canceling Rust GH actions because that changes are missing.

@jmillan jmillan force-pushed the jmillan/do-not-use-json-for-internal-routing branch from 5c81f48 to 6358399 Compare July 20, 2022 10:21
node/src/Consumer.ts Outdated Show resolved Hide resolved
@@ -197,4 +200,16 @@ namespace Channel

this->channel->Send(jsonResponse);
}

const std::string& ChannelRequest::GetNextInternalRoutingId()
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps "Pop" instead of "Get"? A method called "GetXxxx()" should not modify internal state and should be idempotent. "Pop" instead means remove and give me the last item.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are not doing pop. We maintain an internal nextRoutingLevel member that we increase everytime this function is called.

Yes, "GetXxx" should generally not modify the state.

Copy link
Member Author

@jmillan jmillan Jul 20, 2022

Choose a reason for hiding this comment

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

initially we commented about keeping the routing IDs within a std::queue, and pop() as we get the next id. But after implementation it seemed more convoluted than re-using the original std::vector returned from splitting the message, plus this way we can avoid creating a string instance for every call to the function.

@Hartigan
Copy link

What you think about replacing json by protobuf? This way we can save CPU/RAM and generate types for typescript/c++/rust.

@jmillan
Copy link
Member Author

jmillan commented Jul 25, 2022

We are completely removing any data structure for internal.

Data structures for data will be reviewed afterwards.

@nazar-pc nazar-pc mentioned this pull request Aug 18, 2022
ibc added a commit that referenced this pull request Aug 23, 2022
- Replaces PR #870.

### TODO

- There are two TODO comments in CPP files regarding PPID parsing/conversion. We should catch those errors and throw with `MS_THROW_TYPE_ERROR()` instead, otherwise the process will crash since we just expect `MediaSoupErrors`.
- Rust layer.
@ibc
Copy link
Member

ibc commented Aug 23, 2022

Closing in favor of #893

@ibc ibc closed this Aug 23, 2022
ibc added a commit that referenced this pull request Aug 24, 2022
piranna pushed a commit to dyte-in/mediasoup that referenced this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants