search

lml / lev

Ride the rails but don't touch them.
MIT License
2 stars 6 forks source link

Modify Lev::BackgroundJob to not write to store for .find #42

Closed karenc closed 8 years ago

karenc commented 8 years ago

Lev::BackgroundJob.find was writing to store which causes Lev::BackgroundJob.all to be very slow. .find needs to create a new instance of Lev::BackgroundJob but does not need to write to store.

The performance improvement on tutor-server for 1174 background jobs is more than 10 times. Doing Lev::BackgroundJob.all took 1.58s, now it only takes 0.14s.

joemsak commented 8 years ago

The changes look okay to me, I would just address the one comment I had where a boolean argument carries some context with it in callers. Thanks karen :)

karenc commented 8 years ago

@joemsak Can you have a look again? If you have time, try it on your tutor-server ecosystems import page to see if it loads faster?

karenc commented 8 years ago

@jpslav I've looked at your changes in #40 and turns out it doesn't really fix this issue. So I'll have to rebase #40 after this.

jpslav commented 8 years ago

@karenc I just took a look at PR 40 and I think it does fix the issue. The spec from this PR didn't show it exactly because in my PR, I changed new to no longer write to the store. So the 

let!(:job) { described_class.new }

line in the spec didn't really put anything in the store, and then the find! call had to create it, so the spec was failing. I copied your spec over and changed described_class.new to described_class.create and changed find to find! (which is the new method name to show that it will create a job in the store if it isn't found). Here's the commit:

https://github.com/lml/lev/commit/a9a10e4d3d6c32028da72cecf5fdb1e97536a694#diff-662ba3eec78382ea33d1bb9692c61901R90

Does that make sense?

jpslav commented 8 years ago

I added another commit that adds a find method in addition to the existing find! method. find returns nil if the job is not in the store, whereas the bang version creates an unknown status job if not in the store.

https://github.com/lml/lev/commit/d2f1272f6af9599f2a1d17c5e415b57cd20e7369

karenc commented 8 years ago

Thanks for explaining it to me. Let me pick out the bits that we need to fix this slowness issue. (Will open another pull request as the code is going to be very different)