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

Rust: FBS do not use heap to process responses #1194

Merged
merged 5 commits into from
Oct 26, 2023

Conversation

jmillan
Copy link
Member

@jmillan jmillan commented Oct 25, 2023

Avoid heap allocations on response processing. Provides 10% of performance improvement[*].

This is a test bed for rewriting the rest of from_fbs to from_fbs_ref.

NOTE: Once all from_fbs are moved to from_fbs_ref, rename the methods back to from_fbs

[*]: benched producer::dump()

NOTE: I'd like to merge this once approved. Let's have a TODO for making this improvement everywhere once flatbuffers branch is merged. This is totally a non blocker.

NOTE: I'm going to do some checks in Node too. But that's not a blocker for this PR.

Avoid heap allocations. Provides 10% of performance improvement[*].

This is a test bed for rewritting the rest of `from_fbs` to
`from_fbs_ref`.

NOTE: Once all `from_fbs` are moved to `from_fbs_ref`, rename the
methods back to `from_fbs`

[*]: benched producer::dump()
Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

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

Wouldn't it make more sense to not merge this PR in flatbuffers branch but wait and have a separate and complete PR for v3 once flatbuffers lands in v3?

rust/src/router/producer.rs Outdated Show resolved Hide resolved
rust/src/ortc.rs Outdated Show resolved Hide resolved
rust/src/worker/channel.rs Outdated Show resolved Hide resolved
@jmillan
Copy link
Member Author

jmillan commented Oct 25, 2023

Wouldn't it make more sense to not merge this PR in flatbuffers branch but wait and have a separate and complete PR for v3 once flatbuffers lands in v3?

It's a proof of concept that I wanted to try just to see if there was any perf improvement. It turns out that it comes with 10% of improvement so we can say we want it that way. This little PR took me almost a day to write, even though the porting the rest of from_fbs methods will theoretically be faster, it's tedious.

I would like this to be merged, which prepares the land for porting the remaining methods, which could be done in a single PR or in various, since as I say, it takes some time doing this manual stuff and right now I'm reaching my human limit.

@ibc
Copy link
Member

ibc commented Oct 25, 2023

I would like this to be merged, which prepares the land for porting the remaining methods, which could be done in a single PR or in various, since as I say, it takes some time doing this manual stuff and right now I'm reaching my human limit.

Fair.

@ibc
Copy link
Member

ibc commented Oct 25, 2023

Wow, this error in CI is amazing. I've restarted it assuming it is someone's issue.

https://github.com/versatica/mediasoup/actions/runs/6641579661/job/18044437060?pr=1194

npm-scripts [INFO] [worker:build] buildWorker()
npm-scripts [INFO] [worker:build] installMsysMake()
npm-scripts [INFO] [worker:build] executeCmd(): C:\hostedtoolcache\windows\Python\3.9.[13](https://github.com/versatica/mediasoup/actions/runs/6641579661/job/18044437060?pr=1194#step:8:14)\x64\python3.exe worker\scripts\getmake.py --dir="D:\a\mediasoup\mediasoup\worker\out\msys"
Traceback (most recent call last):
  File "D:\a\mediasoup\mediasoup\worker\scripts\getmake.py", line 28, in <module>
    get('https://sourceforge.net/projects/mingw/files/MSYS/Base/make/make-3.81-3/make-3.81-3-msys-1.0.13-bin.tar.lzma/download', '847f0cbbf07135801c8e67bf692d29b1821e816ad828753c997fa869a9b89988')
  File "D:\a\mediasoup\mediasoup\worker\scripts\getmake.py", line 16, in get
    assert hashlib.sha256(data).hexdigest() == digest
AssertionError
npm-scripts [ERROR] [worker:build] executeCmd() failed, exiting: Error: Command failed: C:\hostedtoolcache\windows\Python\3.9.13\x64\python3.exe worker\scripts\getmake.py --dir="D:\a\mediasoup\mediasoup\worker\out\msys"

It's also happening in Windows + Rust:

I--- stderr
  Traceback (most recent call last):
    File "D:\a\mediasoup\mediasoup\worker\scripts\getmake.py", line 26, in <module>
      get('https://sourceforge.net/projects/mingw/files/MSYS/Base/termcap/termcap-0.20050421_1-2/libtermcap-0.20050421_1-2-msys-1.0.13-dll-0.tar.lzma/download', '62b58fe0880f0972fcc84a819265989b02439c1c5185870227bd25f870f7adb6')
    File "D:\a\mediasoup\mediasoup\worker\scripts\getmake.py", line 16, in get
      assert hashlib.sha256(data).hexdigest() == digest
  AssertionError
  thread 'main' panicked at 'Failed to install MSYS/make', worker\build.rs:110:17

@ibc
Copy link
Member

ibc commented Oct 25, 2023

Guys, I hate Python. This is consistently happening despite I restart failing jobs over and over:

https://github.com/versatica/mediasoup/actions/runs/6643130351/job/18049505764?pr=1194

urllib.error.URLError: <urlopen error [WinError 10060] A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond>
npm-scripts [ERROR] [worker:build] executeCmd() failed, exiting: Error: Command failed: C:\hostedtoolcache\windows\Python\3.9.13\x64\python3.exe worker\scripts\getmake.py --dir="D:\a\mediasoup\mediasoup\worker\out\msys"

@jmillan
Copy link
Member Author

jmillan commented Oct 25, 2023

Guys, I hate Python. This is consistently happening despite I restart failing jobs over and over:

I'm not saying this is 100% related but I've found issues happening only in certain branches that don't happen once merged. Windows CI issues are getting very annoying.

@jmillan
Copy link
Member Author

jmillan commented Oct 26, 2023

Concerns noted here have been handled plus ConsumerDump construction has also been reimplemented not to make use of heap. I'll merge once CI passes.

@jmillan jmillan merged commit 3dd69b5 into flatbuffers Oct 26, 2023
32 checks passed
@jmillan jmillan deleted the fbs-response-no-heap branch October 26, 2023 10:52
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