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

Pre-allocate a buffer when getting content #187

Merged
merged 1 commit into from Feb 13, 2018
Merged

Conversation

MasterDuke17
Copy link
Contributor

Instead of growing it with ~=. This reduced the amount of memory used
considerably.

@MasterDuke17
Copy link
Contributor Author

maxrss is reduced to 219.3125MiB when running https://gist.github.com/AlexDaniel/0cdb3211cfcae90c29da9330649171c2 (down from 1500MiB).

@MasterDuke17
Copy link
Contributor Author

Turns out I've mixed up two different changes and I need to get them sorted out before this can be merged.

Instead of growing it with `~=`. This reduces the amount of memory used
considerably.
@MasterDuke17
Copy link
Contributor Author

MasterDuke17 commented Feb 13, 2018

@sergot @ugexe @jonathanstowe any idea what's up with the failing test? It's saying Could not connect socket: Connection refused, so it looks like it might be a problem in https://github.com/sergot/http-useragent/blob/master/t/230-binary-request.t#L40 (and not necessarily my code), but the test passes locally for me.

@AlexDaniel
Copy link
Collaborator

Seems to be ok now?

@sergot
Copy link
Owner

sergot commented Feb 13, 2018

Restarted the build and it passed. 👍

@AlexDaniel
Copy link
Collaborator

Merge it then? :)

@ugexe ugexe merged commit 4e1a238 into sergot:master Feb 13, 2018
@AlexDaniel
Copy link
Collaborator

OK, I hate to say it, but can we revert this please? :)

This is currently making Pastebin::Gist uninstallable because it hangs with 100% CPU on its tests. I've noticed this issue indirectly in whateverable also. Note that HTTP::UserAgent tests are not affected by this regression (with and without NETWORK_TESTING), which means that this should be resolved with tests. Until then a revert will do.

@AlexDaniel
Copy link
Collaborator

See this PR instead of reverting: #188

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.

None yet

4 participants