refactor: memoize record loads in move_(higher|lower)
#415
Conversation
| @@ -1,6 +1,6 @@ | |||
| sqlite: | |||
| adapter: sqlite3 | |||
| database: "file:memdb1?mode=memory&cache=shared" | |||
| database: ":memory:" | |||
There was a problem hiding this comment.
I think the intent of mode=memory is to use an in-memory SQLite database, but that doesn't seem to be working, so this is a functional change (at least, for me).
This isn't an essential part of this PR, but has a couple advantages:
- in-memory tests run faster
- there's no risk of the test suite exiting early and leaving a locked DB file around
Both of these make it easier to run a testing loop with rerun -- bundle exec rake or similar.
There was a problem hiding this comment.
This one comes from here: a070fad
There is a multi-threaded test routine in the suite that tries to generate a real race condition and I think this change was required in order to get that to happen (otherwise the database is single-threaded). That's my understanding at least :D
There's more detail here: 79f56a0
I'm happy to look at removing it provided we don't create a regression in the testing :)
There was a problem hiding this comment.
Hi @jamesdabbs, great to meet you. I'm always keen for help with acts_as_list :) I'm also the maintainer of Ranked Model. I seem to be in the habit of collecting ordering gems :D
Funnily enough, I recently wrote my own little concern as an experiment into a better acts_as_list system with safer database interactions. I wanted something that made it impossible to end up with gaps between positions: https://gist.github.com/brendon/d6cfd60cb5e70dc77a15a2476f04d279
Going forward, I think it'd be cool to go in that kind of direction though there's lots of compatibility cruft that would be required to maintain backward compatibility with acts_as_list as people use it in such varied ways.
| @@ -82,27 +82,27 @@ def insert_at!(position = acts_as_list_top) | |||
|
|
|||
| # Swap positions with the next lower item, if one exists. | |||
| def move_lower | |||
| return unless lower_item | |||
| return unless (other = lower_item) | |||
There was a problem hiding this comment.
Despite 15 odd years experience with Ruby, this one still made me double-take :) It's succinct for sure, but subtle enough that I initially read it as a comparison. I know it's less elegant but I think something like:
other_item = lower_item
return unless other_item
reads clearer. The other option would be wrapping the whole thing in an if.
Is there any kind of style guide for something like this?
My other question is whether ActiveRecord already caches this query? I've not checked, but it should show that in the logs, so while there's a lot of log noise, it's all just hits to the cached result? :)
| @@ -1,6 +1,6 @@ | |||
| sqlite: | |||
| adapter: sqlite3 | |||
| database: "file:memdb1?mode=memory&cache=shared" | |||
| database: ":memory:" | |||
There was a problem hiding this comment.
This one comes from here: a070fad
There is a multi-threaded test routine in the suite that tries to generate a real race condition and I think this change was required in order to get that to happen (otherwise the database is single-threaded). That's my understanding at least :D
There's more detail here: 79f56a0
I'm happy to look at removing it provided we don't create a regression in the testing :)
Heyo 馃憢馃徎 I work at Procore, and we're looking at the performance (in particular - the database query pressure) of
acts_as_list. I think this PR should be a quick win, but I'm hoping we can contribute a number of performance improvements over time. Happy to chat and swap notes if that'd ever be helpful.lower_itemmakes a DB call; memoizing it inmove_lowerallows us to save a few queries. (Sim.higher_itemandmove_higher).