Skip to content

/ servo

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

Read 32kb from the network at a time. #17169

Merged
merged 1 commit into from Jun 6, 2017
Merged

Read 32kb from the network at a time. #17169

merged 1 commit into from Jun 6, 2017

Conversation

@jdm
Copy link
Member

jdm commented Jun 5, 2017 •
edited by larsbergstrom

Now that #16989 has merged, this might actually work.

This change is Reviewable
@jdm
Read 32kb from the network at a time.
Loading status checks…
fe74460
@highfive
Copy link

highfive commented Jun 5, 2017

Heads up! This PR modifies the following files:
@highfive
Copy link

highfive commented Jun 5, 2017

warning Warning warning

  • These commits modify net code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member Author

jdm commented Jun 5, 2017

@bors-servo: try
@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2017

⌛ Trying commit fe74460 with merge dec4902...
bors-servo added a commit that referenced this pull request Jun 5, 2017
@bors-servo
Auto merge of #17169 - servo:jdm-patch-2, r=<try>
Loading status checks…
dec4902 
Read 32kb from the network at a time.

Now that #16989 has merged, this might actually work.
@SimonSapin
Copy link
Member

SimonSapin commented Jun 5, 2017

Looks like read_block doesn’t need to be generic, we might be able to avoid zeroing the buffers and passing uninitialized memory to StreamedResponse::read.
@jdm
Copy link
Member Author

jdm commented Jun 5, 2017

Homu never reported back, but all tests passed.
@jdm
Copy link
Member Author

jdm commented Jun 5, 2017

Having thought about it further, I would prefer not to introduce unsafe code here without strong motivation.
@jdm
Copy link
Member Author

jdm commented Jun 5, 2017

r? @SimonSapin
@highfive highfive assigned SimonSapin and unassigned pcwalton Jun 5, 2017
@SimonSapin
Copy link
Member

SimonSapin commented Jun 5, 2017

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2017

📌 Commit fe74460 has been approved by SimonSapin
@bors-servo
Copy link
Contributor

bors-servo commented Jun 6, 2017

⌛ Testing commit fe74460 with merge 6fe0e30...
bors-servo added a commit that referenced this pull request Jun 6, 2017
@bors-servo
Auto merge of #17169 - servo:jdm-patch-2, r=SimonSapin
Loading status checks…
6fe0e30 
Read 32kb from the network at a time.

Now that #16989 has merged, this might actually work.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17169)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 6, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: SimonSapin
Pushing 6fe0e30 to master...
@bors-servo bors-servo merged commit fe74460 into master Jun 6, 2017
4 checks passed
4 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details
@SimonSapin SimonSapin deleted the jdm-patch-2 branch Jun 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SimonSapin SimonSapin

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@jdm @highfive @bors-servo @SimonSapin @pcwalton
You can’t perform that action at this time.