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

chore: autoload TwiML to save memory on load #498

Merged
merged 2 commits into from Apr 3, 2020

Conversation

philnash
Copy link
Contributor

@philnash philnash commented Mar 11, 2020

Works towards fixing #396.

Others are reporting that twilio-ruby takes a lot of memory. My belief is that this is because it is a large library that covers an ever increasing API surface. The issue is not necessarily in that the library is so large, but that the entire thing is loaded in one go with requires for days.

I am going to send a few PRs that update our use of require to autoload where appropriate. autoload only loads the file when the constant (class/module) is used. In this first PR, for those who include the library but don't use TwiML, the files governing TwiML creation will not be loaded into memory. For those that do use TwiML the behaviour will be unchanged. Notably this also moves requiring nokogiri to the TwiML files too. This will not affect a Rails user, but users of frameworks which don't include nokogiri should notice a difference.

This first PR will not significantly reduce the memory usage, but it is a start and smaller PRs are easier to review.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the master branch
  • I have added tests that prove my fix is effective or that my feature works (all existing tests pass)
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

@philnash philnash mentioned this pull request Mar 11, 2020
8 tasks
Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

Cross-posting this:

Changes look good to me. The only concern that I have (which is minor) is the negative impact of lazy-loading. For example, I previously expected a longer start-up time but quicker run-time (on first invocation). Thoughts on making this opt-in? This was done recently for the twilio-node lib where lazy-load is off by default with an opt-in.

@childish-sambino childish-sambino added the status: waiting for feedback waiting for feedback from the submitter label Mar 12, 2020
@philnash
Copy link
Contributor Author

philnash commented Apr 3, 2020

Changes look good to me. The only concern that I have (which is minor) is the negative impact of lazy-loading. For example, I previously expected a longer start-up time but quicker run-time (on first invocation). Thoughts on making this opt-in? This was done recently for the twilio-node lib where lazy-load is off by default with an opt-in.

The more I think about this, the more I think that lazy loading can only have a positive effect for the majority of cases. Given that most applications are not using the full suite of APIs once this can all be split out the loading times and memory impact of the library will be much better and then the first invocation time won't be significantly impacted because loading extra files on the first invocation will be much smaller than loading the whole library too.

Also, we can use things like TwiML and JWT as test cases for this because they are easy to split out like this (these are not complicated PRs) and if users find that they have issues when they use TwiML or Access Tokens in their app we can roll back.

Finally, I'd worry that offering this as opt in would both complicate the code unnecessarily and lead to a number of developers missing out on the memory savings because it would be easy to miss.

What do you think @childish-sambino?

@childish-sambino
Copy link
Contributor

Works for me!

@childish-sambino childish-sambino merged commit d209e35 into twilio:master Apr 3, 2020
@childish-sambino childish-sambino removed the status: waiting for feedback waiting for feedback from the submitter label Apr 3, 2020
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

2 participants