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

Revert "perf(ext/websocket): optimize socket.send (#16320)" #17480

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Revert "perf(ext/websocket): optimize socket.send (#16320)" #17480

merged 1 commit into from
Jan 20, 2023

Conversation

crowlKats
Copy link
Member

This reverts commit 36307c4

littledivy
littledivy reviewed Jan 20, 2023
View reviewed changes
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

Why?

@aapoalas
Copy link
Collaborator

aapoalas commented Jan 20, 2023 •
edited
Loading

Why?

This and it's friends:
#17332

bartlomieju
bartlomieju approved these changes Jan 20, 2023
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

@crowlKats
Copy link
Member Author

Why?

this was supposed to be reverted months ago, but for some reason the revert never landed on main.

@crowlKats crowlKats merged commit 638b6ef into denoland:main Jan 20, 2023
@crowlKats crowlKats deleted the revert_16320 branch January 20, 2023 16:20
@littledivy
Copy link
Member

Ok but we don't have a reproduction for the issue and no failing test case. How can we get this relanded? This revert blocks #16315

@aapoalas
Copy link
Collaborator

Ok but we don't have a reproduction for the issue and no failing test case. How can we get this relanded? This revert blocks #16315

There are a few reproductions linked in the issue I linked, I believe. There is also a ticket asking for more tests for WS to avoid these reappearing, like they've done in this case.

@sgwilym sgwilym mentioned this pull request Jan 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@littledivy littledivy littledivy left review comments

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@crowlKats @aapoalas @littledivy @bartlomieju