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

Support serenity simd_json #105

Merged
merged 21 commits into from
Jul 25, 2022
Merged

Conversation

vicky5124
Copy link
Contributor

modified:   src/shards.rs

arqunis and others added 8 commits October 12, 2021 14:43
…erenity-rs#6)

* Depend on Serenity's `next` branch on Songbird's `next` branch

* Update the examples to use the `next` branch
Serenity's cache design has changed, this (finally) prevents the examples from failing to compile on CI.
Adds generics to any `Id` types on `Call`. Also includes the overlooked `Songbird::get_or_insert`.

Closes serenity-rs#94. Tested using `cargo make ready`.
This matches a recent serenity change where `ClientBuilder` no longer has an explicit lifetime parameter.

This was tested using `cargo make ready`.
	modified:   src/shards.rs
	modified:   src/handler.rs
	modified:   src/handler.rs
	modified:   src/shards.rs
Copy link
Member

@FelixMcFelix FelixMcFelix left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, just one minor nit regarding import formatting. I had a quick look at the repo and it seems that voice-model is untouched, so I think this is the extent of everything you need to change. :)

src/shards.rs Outdated Show resolved Hide resolved
@vicky5124 vicky5124 marked this pull request as draft December 7, 2021 16:33
@vicky5124
Copy link
Contributor Author

sorry! should have marked this as a draft, as there's still way more files to modify, and things that need to be fixed on serenity first before this can work and be merged.
See serenity-rs/serenity#1613

About the import order, cargo-fmt made it that way.

	modified:   ../../../Cargo.toml
	modified:   ../../../src/driver/mod.rs
	modified:   ../../../src/input/dca.rs
	modified:   ../../../src/input/error.rs
	modified:   ../../../src/input/ffmpeg_src.rs
	modified:   ../../../src/input/metadata.rs
	modified:   ../../../src/input/ytdl_src.rs
	modified:   ../../../src/ws.rs
	modified:   Cargo.toml
	modified:   src/driver/mod.rs
	modified:   src/input/dca.rs
	modified:   src/input/error.rs
	modified:   src/input/ffmpeg_src.rs
	modified:   src/input/metadata.rs
	modified:   src/input/ytdl_src.rs
	modified:   src/ws.rs
	modified:   src/driver/mod.rs
	modified:   src/input/dca.rs
	modified:   src/input/ffmpeg_src.rs
	modified:   src/input/metadata.rs
	modified:   src/input/ytdl_src.rs
	modified:   src/tracks/error.rs
	modified:   src/ws.rs
	modified:   Cargo.toml
	modified:   examples/serenity/voice_receive/Cargo.toml
	modified:   src/input/metadata.rs
	modified:   src/ws.rs
	modified:   examples/serenity/voice_receive/Cargo.toml
	modified:   README.md
	modified:   .github/workflows/ci.yml
@vicky5124 vicky5124 marked this pull request as ready for review December 7, 2021 18:28
	modified:   .github/workflows/ci.yml
	new file:   .cargo/config.toml
Copy link
Member

@FelixMcFelix FelixMcFelix left a comment

Choose a reason for hiding this comment

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

Currently relies on serenity's simdjson support being fixed before I can end-to-end test this.

Otherwise compiles fine, comments are inline with respect to all driver-related JSON parsing.

@@ -0,0 +1,2 @@
[build]
rustflags = ["-C", "target-cpu=haswell"]
Copy link
Member

Choose a reason for hiding this comment

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

Why not native?

Can this also be moved to CI such that it's only invoked for the simd-json build task? I'm unsure if the spurious MacOS SIGILL originates from this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, unless you do CI with Arm, this target-cpu will work just fine.
Haswell was chosen over native as I've seen a few people mention it's less likely for it to fail CI due to "Illegal Instruction"; MacOS failed because there's still a chance it can fail from that, serenity has had this issue for quite a while now, and it's on Actions for it to be fixed.

Moving this to only the SIMD check would make it more likely to pass, and I'll do it when I have time for it, but it's hard to test CI when it's blocked behind it being accepted due to this being my first PR.

src/input/dca.rs Outdated
serenity::json::prelude::from_slice::<DcaMetadata>(raw_json.as_mut_slice())
.map_err(DcaError::InvalidMetadata)?
.into();
#[cfg(not(feature = "serenity"))]
Copy link
Member

@FelixMcFelix FelixMcFelix Dec 13, 2021

Choose a reason for hiding this comment

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

General feedback for all other driver/input uses: wouldn't it make more sense to use the simd-json library ourselves if that feature is selected? That way we can apply this to twilight too, and use serenity::json::* only where we interface with serenity in the gateway portions. Twilight v0.8 now exposes their own JSON type via twilight_gateway::shard::raw_message::Message from what I can see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I somewhat planned for this, but I have 0 idea how twilight manages SIMD; I will not make a PR of something I'm completely unfamiliar with. If someone else wants to implement the SIMD feature for twilight, go for it.

@vicky5124
Copy link
Contributor Author

Serenity has fixed it's SIMD issues, this PR is ready to be tested.

@FelixMcFelix
Copy link
Member

Tried again there, still getting plenty of messages like below after a cargo update:

Jan 08 17:07:25.284  WARN run:recv_event:handle_event{event=Err(SimdJson(Error { index: 0, character: '💩', error: Serde("event GuildCreate: Serde(\"Serde(\\\"Serde(\\\\\\\"invalid type: string \\\\\\\\\\\\\\\"925690819955023872\\\\\\\\\\\\\\\", expected tuple struct MessageId\\\\\\\") at character 0 ('💩')\\\") at character 0 ('💩')\") at character 0 ('💩')") }))}: serenity::gateway::shard: [Shard [0, 1]] Unhandled error: SimdJson(Error { index: 0, character: '�'', error: Serde("event GuildCreate: Serde(\"Serde(\\\"Serde(\\\\\\\"invalid type: string \\\\\\\\\\\\\\\"925690819955023872\\\\\\\\\\\\\\\", expected tuple struct MessageId\\\\\\\") at character 0 ('💩')\\\") at character 0 ('💩')\") at character 0 ('�'')") })

@nickelc
Copy link

nickelc commented Jul 5, 2022

Tried again there, still getting plenty of messages like below after a cargo update:

Jan 08 17:07:25.284  WARN run:recv_event:handle_event{event=Err(SimdJson(Error { index: 0, character: '💩', error: Serde("event GuildCreate: Serde(\"Serde(\\\"Serde(\\\\\\\"invalid type: string \\\\\\\\\\\\\\\"925690819955023872\\\\\\\\\\\\\\\", expected tuple struct MessageId\\\\\\\") at character 0 ('💩')\\\") at character 0 ('💩')\") at character 0 ('💩')") }))}: serenity::gateway::shard: [Shard [0, 1]] Unhandled error: SimdJson(Error { index: 0, character: '�'', error: Serde("event GuildCreate: Serde(\"Serde(\\\"Serde(\\\\\\\"invalid type: string \\\\\\\\\\\\\\\"925690819955023872\\\\\\\\\\\\\\\", expected tuple struct MessageId\\\\\\\") at character 0 ('💩')\\\") at character 0 ('💩')\") at character 0 ('�'')") })

I think that was an issue in simd-json and should be fixed now.

@FelixMcFelix
Copy link
Member

I'll manually fixup and test after #89 is merged (i.e., after a cleanup pass). Apologies that this has been left for quite so long.

@FelixMcFelix
Copy link
Member

Okay, I've fixed this up to work without serenity now (and I believe JSON decode in DCA format and YoutubeDL handling should be accelerated too). This might take some fiddling to make sure the CI plays nice -- I've tried to make sure that RUSTFLAGS are only ever set on the Linux/simd target (since MacOS breaks routinely).

@FelixMcFelix FelixMcFelix merged commit 2774d3e into serenity-rs:next Jul 25, 2022
FelixMcFelix added a commit that referenced this pull request Jul 26, 2022
This PR adds support for the simd-json library whenever decoding or encoding JSON responses. This may be enabled independently of serenity and twilight support for SIMD acceleration.

Co-authored-by: Kyle Simpson <kyleandrew.simpson@gmail.com>
@FelixMcFelix FelixMcFelix added enhancement New feature or request driver Relates to the driver or one of its sub-tasks. gateway Relates to the gateway feature (Discord's gateway integration). labels Jul 27, 2022
FelixMcFelix added a commit to FelixMcFelix/songbird that referenced this pull request Nov 20, 2023
This PR adds support for the simd-json library whenever decoding or encoding JSON responses. This may be enabled independently of serenity and twilight support for SIMD acceleration.

Co-authored-by: Kyle Simpson <kyleandrew.simpson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
driver Relates to the driver or one of its sub-tasks. enhancement New feature or request gateway Relates to the gateway feature (Discord's gateway integration).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants