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

empty the raw response string after parsing #1588

Merged
merged 1 commit into from Jan 9, 2019

Conversation

Projects
None yet
3 participants
@dbu
Copy link
Contributor

dbu commented Jan 8, 2019

when a query yields a large response, duplicating the data between string and parsed array duplicates the memory consumption.
once $this->_response is set, $this->_responseString is not accessed anywhere in the code anymore.

@p365labs

This comment has been minimized.

Copy link
Collaborator

p365labs commented Jan 8, 2019

@dbu that's really interesting.. I've had a look at the code and _responseString is never accessed after the use in getData(). Does it really created a problem in memory consumption ? Glad that you discovered it, maybe this could save life to lot of people :) Do you think that this could be the only way to solve it ?

@dbu

This comment has been minimized.

Copy link
Contributor Author

dbu commented Jan 8, 2019

we have queries where the raw json is several megabytes, so yes, duplicating that is a cost.

another option would be to parse in the constructor and never even set the raw string. that would mean a performance penalty if the result is not used at all, not sure if that is likely to happen. might be for changing requests that the http status is enough to know it worked...

this change looked like the smallest impact where we can be sure there is no negative side effect in any circumstate.

@p365labs

This comment has been minimized.

Copy link
Collaborator

p365labs commented Jan 8, 2019

I agree with you. I think we can proceed this way right now. @ruflin what do you think on this ?

@ruflin

This comment has been minimized.

Copy link
Owner

ruflin commented Jan 9, 2019

👍 on the change. A changelog entry would be nice.

empty the raw response string after parsing
when a query yields a large response, duplicating the data between string and parsed array duplicates the memory consumption.
once $this->_response is set, $this->_responseString is not accessed anywhere in the code anymore.

@dbu dbu force-pushed the dbu:patch-1 branch from 0165826 to 5a8f015 Jan 9, 2019

@dbu

This comment has been minimized.

Copy link
Contributor Author

dbu commented Jan 9, 2019

added a changelog entry. ok like this?

@ruflin

ruflin approved these changes Jan 9, 2019

Copy link
Owner

ruflin left a comment

LGTM, assuming CI goes green ;-)

@p365labs p365labs merged commit 924c8f9 into ruflin:master Jan 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dbu dbu deleted the dbu:patch-1 branch Jan 9, 2019

dbu added a commit to dbu/Elastica that referenced this pull request Jan 9, 2019

ruflin added a commit that referenced this pull request Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment