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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃洜 update lodash,knex and bookshelf to latest versions #7402

Merged

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Sep 19, 2016

refs #7189

  • we had a memory leak after upgrading to bookshelf 0.10.1
  • bookshelf has fixed the memory leak in 0.10.2

@ErisDS
Copy link
Member

ErisDS commented Sep 19, 2016

3 requests per second to a Ghost server which has lot of data in the database.
The memory with latest lodash, latest bookshelf and latest knex grows to a max memory of ~300mb and always falls down to 130-140mb.

Is that the same or different to the previous test? Can you maybe share a graph of both?

If I am remembering correctly, this same test did not reproduce the memory leak last time, and so this is not a good test to base an conclusion on?

I now have access to a trace account, so maybe we can setup a longer running test tomorrow ?

@kirrg001
Copy link
Contributor Author

kirrg001 commented Sep 19, 2016

Is that the same or different to the previous test? Can you maybe share a graph of both?

Same. I don't have a graph.

If I am remembering correctly, this same test did not reproduce the memory leak last time, and so this is not a good test to base an conclusion on?

I was able to reproduce the memory leak with this exact test suite. And now it's gone with the new knex bump.

I now have access to a trace account, so maybe we can setup a longer running test tomorrow ?

If you want sure.

My ghost server is running now an hour and memory is good.

@ErisDS
Copy link
Member

ErisDS commented Sep 19, 2016

When I asked you if it was the same, I meant the results, not the test itself (you stated in your PR that the test was the same test). I'm asking how these results compare to the results you saw last time? I want to see side-by-side results so that I can understand the difference, if there is one.

@kirrg001
Copy link
Contributor Author

kirrg001 commented Sep 19, 2016

I can make graphs tomorrow 馃憤 with knex 0.10 (which we are using now), knex 0.11 (which had a memory leak) and knex 0.12. (which solved the memory leak)

@ErisDS
Copy link
Member

ErisDS commented Sep 19, 2016

Either a graph or a table would be fab.

@kirrg001 kirrg001 changed the title 馃洜 update lodash,knex and bookshelf to latest versions [WIP] 馃洜 update lodash,knex and bookshelf to latest versions Sep 20, 2016
@kirrg001
Copy link
Contributor Author

It turned out that knex 0.12 is not solving the memory problems.
I did another measuring this morning. Here are the results (tested with lodash 3 and 4, it doesn't matter)


Bookshelf 0.9.x
Knex 0.10.x

Bookshelf 0.9.x
Knex 0.12.x

1
(both setups result in the same graph)


Bookshelf 0.10.x
Knex 0.12.x

Bookshelf 0.10.x
Knex 0.10.x

2
(both setups result in the same graph)

Looks like the bookshelf version 0.10.x is causing the problems.

@tgriesser
Copy link
Contributor

I'll need to take a look to see what's happening here

@tgriesser
Copy link
Contributor

@kirrg001 is there a step-by-step I can follow to reproduce those graphs and get those results?

@tgriesser
Copy link
Contributor

Just in looking through the commit log I believe the issue likely has to do with the __super__ reference in .extends in this commit.

If it is indeed the source of issue I can go ahead and revert the portions that change asap, will try and take a look a little later.

@kirrg001
Copy link
Contributor Author

@tgriesser Thanks for getting involved.

To reproduce the problem, you need to checkout this branch and setup a ghost blog with a bigger database (i had 20 posts in my database). Then you can trigger some curl commands from shell and watch the memory. Write me a PM in slack, if you need help.

see also #7189

@tgriesser
Copy link
Contributor

This is fixed in Bookshelf 0.10.2 馃帀 - Thanks @kirrg001 for the help in profiling / tracking this one down!

@kirrg001 kirrg001 force-pushed the tooling/latest-lodash-bookshelf-knex branch from 3706448 to a3cbbed Compare September 23, 2016 05:08
@kirrg001 kirrg001 changed the title [WIP] 馃洜 update lodash,knex and bookshelf to latest versions 馃洜 update lodash,knex and bookshelf to latest versions Sep 23, 2016
@kirrg001
Copy link
Contributor Author

bookshelf0 10 2

I can confirm: the memory leak is fixed in 0.10.2.
Thank you @tgriesser!

@kirrg001 kirrg001 added this to the 1.0.0-alpha.2 milestone Sep 23, 2016
@kirrg001
Copy link
Contributor Author

ready for review && merge

@ErisDS
Copy link
Member

ErisDS commented Sep 26, 2016

@kirrg001 this needs rebasing please :)

@kirrg001
Copy link
Contributor Author

@ErisDS 馃憤 馃憤 didn't see, will do now

refs TryGhost#7189
- we had a memory leak after upgrading to knex 0.11.x
- knex has published a new version 0.12.x
- the memory leak does not longer exists
- knex has reverted their pool logic, see knex/knex#1665
@kirrg001 kirrg001 force-pushed the tooling/latest-lodash-bookshelf-knex branch from a3cbbed to 6c100a1 Compare September 26, 2016 08:21
@kirrg001
Copy link
Contributor Author

@ErisDS did

@ErisDS ErisDS merged commit 6d092ad into TryGhost:master Sep 26, 2016
@ErisDS ErisDS added the LTS label Sep 29, 2016
@ErisDS
Copy link
Member

ErisDS commented Sep 29, 2016

I would like this same PR made to the lts branch if possible? I can do it, but I figured you might want to 馃槈

@kirrg001
Copy link
Contributor Author

@ErisDS sure will create

@ErisDS ErisDS removed their assignment Jun 22, 2021
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

3 participants