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

Reduce memory footprint #182

Merged
merged 26 commits into from Jan 16, 2019

Conversation

Projects
None yet
4 participants
@mastef
Copy link
Contributor

mastef commented Nov 1, 2017

This helped me with #140

Basically we remove the long edit and self links from each cell, and just construct them if necessary. We still save them if they don't match our pattern, so that's a fallback for API/URL changes.

We also unset the raw string return body, and don't pass it on to the callback ( this seemed like an undocumented feature - maybe used previously for debugging purposes? )

Managed to reduce the memory footprint drastically already on sheets with ~25k cells.

mastef added some commits Nov 1, 2017

delete response body, since we have a parsed result already
Also the callback does not have a documented "body" param, might have been used for debugging, however it increases memory footprint for huge sheets.
add getters for "edit" and "self" links, and don't set them if they m…
…atch our pattern

They are usually very long strings per cell, which can add up on big sheets
@theoephraim

This comment has been minimized.

Copy link
Owner

theoephraim commented Nov 3, 2017

Overall looks good! I can't remember exactly why the xml is being passed around. I do know that the API is very strict/picky about the XML it accepts, so when making further requests I ended up modifying the string of xml they gave and passing it back. Any attempts I made at deconstructing/reconstructing the xml always failed.

Could you squash these commits into a single commit?

@mastef

This comment has been minimized.

Copy link
Contributor Author

mastef commented Nov 3, 2017

I found out the cases where the xml was needed, and added them back in in 689c56c - they were only necessary for a few cases, but removing them from cells lookup is still possible by counting the callback params. That saves memory

As to merging the commits, not sure - is it necessary? I have already followup commits pending in my branch - mainly the change towards a prototype methods version of SpreadsheetCells which gives 50%+ memory savings as not every cell has their own methods attached to it ( see #183 )

I could add that to this PR if you'd like, or create a separate one for it

@mastef

This comment has been minimized.

Copy link
Contributor Author

mastef commented Mar 8, 2018

@theoephraim when you merge a PR you can squash the commits into one

@mastef

This comment has been minimized.

Copy link
Contributor Author

mastef commented Mar 8, 2018

I've added the other memory related commits as I needed them in my branch.

This moves the SpreadsheetCell methods to the prototype. So when we have a spreadsheet with 50k cells, we don't create 50k objects with their own 15 methods ( 750,000 methods ), but instead the 50k objects share just the same 15 methods.

@macrozone

This comment has been minimized.

Copy link

macrozone commented May 13, 2018

@mastef i tried your fork, but SpreadsheetCell.prototype.save throws spreadsheet is not defined. I would suggest to use eslint to catch these kinds of errors

@mastef

This comment has been minimized.

Copy link
Contributor Author

mastef commented May 14, 2018

@macrozone Thanks for this. I've resolved this and a few smaller issues after running it through eslint. If you have a better config let me know.

@macrozone

This comment has been minimized.

Copy link

macrozone commented May 14, 2018

awesome :-D

will check again

@mastef

This comment has been minimized.

Copy link
Contributor Author

mastef commented May 14, 2018

mocha seems to pass as well - 81 passing, 1 pending :

  Row-based feeds
    fetching rows
      - supports `reverse` option

But I don't think that this is related to this fork?

@mastef

This comment has been minimized.

Copy link
Contributor Author

mastef commented Dec 12, 2018

@theoephraim let me know if you can merge this now?

@quesurifn

This comment has been minimized.

Copy link

quesurifn commented Jan 11, 2019

@theoephraim Can you merge this? Having memory issues on prod. @mastef Maybe create a fork and publish to NPM with your commits?

@mastef

This comment has been minimized.

Copy link
Contributor Author

mastef commented Jan 11, 2019

@quesurifn I think you can try my branch with npm install git+https://git@github.com/mastef/node-google-spreadsheet.git

@quesurifn

This comment has been minimized.

Copy link

quesurifn commented Jan 11, 2019

@mastef

This comment has been minimized.

Copy link
Contributor Author

mastef commented Jan 11, 2019

I've been using it in production for tens of thousands of lambda jobs

@quesurifn

This comment has been minimized.

Copy link

quesurifn commented Jan 11, 2019

@mastef

This comment has been minimized.

Copy link
Contributor Author

mastef commented Jan 11, 2019

If somebody could tell me how I can add the latest commits that happened on this branch to my fork, then I could bring it up-to-date as well, as it's ~6 commits behind
image

@quesurifn

This comment has been minimized.

Copy link

quesurifn commented Jan 11, 2019

@mastef Not sure. Can you pull origin somehow? @theoephraim Please please please can you gear this up for release.

@theoephraim

This comment has been minimized.

Copy link
Owner

theoephraim commented Jan 11, 2019

hey guys - yes ill get it merged in and re-release ASAP - prob tonight or tomorrow!

@quesurifn

This comment has been minimized.

Copy link

quesurifn commented Jan 11, 2019

@theoephraim Thank you!!! Prefect too! If you do that I can update for Monday release.

@mastef

This comment has been minimized.

Copy link
Contributor Author

mastef commented Jan 15, 2019

@theoephraim theoephraim merged commit 969b25e into theoephraim:master Jan 16, 2019

@theoephraim

This comment has been minimized.

Copy link
Owner

theoephraim commented Jan 16, 2019

done! sorry it took so long. I'm going to see if I can find a little time to clean the code up as well, set up linting, etc...

@quesurifn

This comment has been minimized.

Copy link

quesurifn commented Jan 16, 2019

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