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

Write data into socket without rewriting obj #134

Merged
merged 6 commits into from
Oct 23, 2018

Conversation

webknjaz
Copy link
Member

@webknjaz webknjaz commented Oct 20, 2018

  • What kind of change does this PR introduce?

    • bug fix
    • feature
    • docs update
    • tests/coverage improvement
    • refactoring
    • other
  • What is the related issue number (starting with #)

Fix #133

  • What is the current behavior? (You can also link to an open issue here)

slow down because of data var rewriting

  • What is the new behavior (if this is a feature change)?

N/A

  • Other information:

  • Checklist:

    • I think the code is well written
    • I wrote good commit messages
    • I have squashed related commits together after the changes have been approved
    • Unit tests for the changes exist
    • Integration tests for the changes exist (if applicable)
    • I used the same coding conventions as the rest of the project
    • The new code doesn't generate linter offenses
    • Documentation reflects the changes
    • The PR relates to only one subject with a clear title
      and description in grammatically correct, complete sentences

This change is Reviewable

@webknjaz webknjaz added the bug Something is broken label Oct 20, 2018
@webknjaz webknjaz requested a review from jaraco October 20, 2018 23:05
@nav-agarwal

This comment has been minimized.

1 similar comment
@nav-agarwal

This comment has been minimized.

@nav-agarwal

This comment has been minimized.

@webknjaz

This comment has been minimized.

@webknjaz

This comment has been minimized.

1 similar comment
@webknjaz

This comment has been minimized.

@webknjaz

This comment has been minimized.

@nav-agarwal
Copy link

This fix still won't help in improving performance as strings are immutable and they are "passed by value" and hence it will still lead to copy of string. Check below sample code to see performance impact of copying whole buffer.

import time

data = "a" * 1024 * 1024 * 200
packet_sz = 16 * 1024
bytes_sent = 0


def dummy_send(data):
    time.sleep(0.0001)  # for write to socket
    return


while data:
    dummy_send(data[bytes_sent:])
    bytes_sent += packet_sz

@webknjaz
Copy link
Member Author

@nav-agarwal GitHub is behaving inconsistently today. Could you please verify that you saw my memoryview commit?

@codecov

This comment has been minimized.

@webknjaz

This comment has been minimized.

@webknjaz webknjaz force-pushed the bugfix/makefile-no-copy-chunks-iter branch from 51ff0a7 to 807aebe Compare October 22, 2018 23:22
@webknjaz webknjaz force-pushed the bugfix/makefile-no-copy-chunks-iter branch from 807aebe to 541829d Compare October 22, 2018 23:50
@webknjaz
Copy link
Member Author

@nav-agarwal memoryview/buffer should fix this now.

@webknjaz webknjaz merged commit bbc8d95 into master Oct 23, 2018
@nav-agarwal
Copy link

I am not getting enought time to test it. But using memoryview might land us into trouble because of pyopenssl limitation. Read more here urllib3/urllib3#717

@webknjaz
Copy link
Member Author

webknjaz commented Nov 1, 2018

@nav-agarwal that's a curious find.. we could do .to_bytes() there as well, if needed.

@webknjaz
Copy link
Member Author

webknjaz commented Nov 1, 2018

Oh, and we could probably use smaller chunks as urllib3 does...

@nav-agarwal
Copy link

yes... something like data[total_sent:total_sent + SSL_WRITE_BLOCKSIZE]

@webknjaz webknjaz mentioned this pull request Nov 1, 2018
15 tasks
@webknjaz
Copy link
Member Author

webknjaz commented Nov 1, 2018

@nav-agarwal #137 should do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow writes for large response body for HTTPS connection
2 participants