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

Save HibernatableWebSocketEventInfo events by value #3303

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

danlapid
Copy link
Collaborator

@danlapid danlapid commented Jan 8, 2025

These events are so small...

  struct Message final {};
  struct Close final {
    uint16_t code;
    bool wasClean;
  };
  struct Error final {};

That there shouldn't even be any performance penalty to saving them by value rather than by reference.
The nice thing about it is we don't need to save bare references that can easily cause UB or worse (such as a bug we're trying hopelessly to debug atm).

@danlapid danlapid requested review from a team as code owners January 8, 2025 00:39
@danlapid danlapid enabled auto-merge January 8, 2025 00:47
@anonrig
Copy link
Member

anonrig commented Jan 8, 2025

The nice thing about it is we don't need to save bare references that can easily cause UB or worse (such as a bug we're trying hopelessly to debug atm).

100 percent. I raise you and say: "not unless we use shared_ptr"

@danlapid
Copy link
Collaborator Author

danlapid commented Jan 8, 2025

The nice thing about it is we don't need to save bare references that can easily cause UB or worse (such as a bug we're trying hopelessly to debug atm).

100 percent. I raise you and say: "not unless we use shared_ptr"

Yeah shared ptr is great but for these structures specifically it’s probably cheaper to just copy them 😅

@anonrig
Copy link
Member

anonrig commented Jan 8, 2025

Yeah shared ptr is great but for these structures specifically it’s probably cheaper to just copy them 😅

Or I can raise one more time and mention reference counting through kj. (I'm so happy to show off my KJ knowledge. Mom and dad are proud)

@danlapid danlapid merged commit ffd99ca into main Jan 8, 2025
15 checks passed
@danlapid danlapid deleted the dlapid/fix_HibernatableWebSocketEventInfo_lifetimes branch January 8, 2025 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants