Skip to content

src,stream: Improve WriteString#51155

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
Merged
nodejs-github-bot merged 4 commits into from
Dec 30, 2023
Merged

src,stream: Improve WriteString #51155

nodejs-github-bot merged 4 commits into from
Dec 30, 2023

Conversation

@ywave620
Copy link
Contributor

@ywave620ywave620 commented Dec 14, 2023

Introduce HasDoTryWrite in order to eliminate the unnecessary memory copy in WriteString for StreamBase not overriding DoTryWrite, e.g. TLSWrap, HTTP2Session

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Dec 14, 2023
@ywave620
src,stream: improve WriteString
6d48cec
Introduce HasDoTryWrite and make use of it in WriteString
tniessen
tniessen reviewed Dec 15, 2023
View reviewed changes
@ywave620
Copy link
ContributorAuthor

ywave620 commented Dec 18, 2023

@tniessen Could you rerun the ci :)

@ywave620
Copy link
ContributorAuthor

ywave620 commented Dec 21, 2023

Could anyone help push this forward? Actually, I think this is a non trivial change, given that it removes an unnecessary memory copy for the very hot case, TLSWrap::WriteString

@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 24, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/56516/

lpinca
lpinca approved these changes Dec 25, 2023
View reviewed changes
Copy link
Member

@lpincalpinca left a comment

Choose a reason for hiding this comment

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

RSLGTM

@lpincalpinca added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 25, 2023
@aduh95aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 30, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 30, 2023
@nodejs-github-botnodejs-github-bot merged commit 461ffca into nodejs:mainDec 30, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 30, 2023

Landed in 461ffca

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@ywave620@RafaelGSS
src,stream: improve WriteString
e7925e6
Introduce HasDoTryWrite and make use of it in WriteString PR-URL: #51155 Reviewed-By: Luigi Pinca <[email protected]>
@RafaelGSSRafaelGSS mentioned this pull request Jan 2, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@ywave620@richardlau
src,stream: improve WriteString
8e5e5b7
Introduce HasDoTryWrite and make use of it in WriteString PR-URL: #51155 Reviewed-By: Luigi Pinca <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@ywave620@richardlau
src,stream: improve WriteString
b616f6f
Introduce HasDoTryWrite and make use of it in WriteString PR-URL: #51155 Reviewed-By: Luigi Pinca <[email protected]>
@richardlaurichardlau mentioned this pull request Mar 25, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessentniessentniessen left review comments

@lpincalpincalpinca approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ywave620@nodejs-github-bot@lpinca@tniessen@aduh95