Bug 13719 - Make Koha::Objects store list of resultant objects on an as needed basis
Summary: Make Koha::Objects store list of resultant objects on an as needed basis
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 13019
Blocks: 13726
  Show dependency treegraph
 
Reported: 2015-02-17 18:34 UTC by Kyle M Hall
Modified: 2016-12-05 21:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13719 - Store list of objects as needed (2.52 KB, patch)
2015-02-17 18:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13719 - Make Koha::Objects store list of resultant objects on an as needed basis (3.35 KB, patch)
2015-02-19 11:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13719: Make Koha::Objects store list of resultant objects on an as needed basis (3.44 KB, patch)
2015-12-18 18:36 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2015-02-17 18:34:02 UTC

    
Comment 1 Kyle M Hall 2015-02-17 18:34:31 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2015-02-18 13:21:23 UTC
Kyle, what would "Store list of objects" mean? I'm sorry but it is too generic for my idiomatic limitations!
Comment 3 Kyle M Hall 2015-02-19 11:05:06 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> Kyle, what would "Store list of objects" mean? I'm sorry but it is too
> generic for my idiomatic limitations!

Sorry, I was just posting the code to get it out there. I will be writing up a better explanation and a test plan shortly!
Comment 4 Kyle M Hall 2015-02-19 11:16:43 UTC
We can both simplify and improve the functioning of Koha::Objects by removing out reliance on DBIC for set iteration ( first(), next(), reset(), etc ). The problem is that DBIC destroys and refetches results every time reset() is called. For example, take the following code:

my $borrowers = Koha::Borrowers->search({ firstname => 'Kyle' }).
my $kyle1 = $borrowers->next();
$borrowers->reset();
my $kyle2 = $borrowers->next();

In this case, we would expect $kyle1 and $kyle2 to refer to the exact same object in memory, but they do *not*. This is simply a limitation of DBIx::Class.

However, by handling this issue ourselves, we not only solve the problem, but I believe we also reduce the complexity of our code.

This is all accomplished without changing the external behavior of the Koha::Objects module.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Borrowers.t
Comment 5 Kyle M Hall 2015-02-19 11:21:34 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2015-06-22 03:55:15 UTC
Couldn't this be a memory hog?
Comment 7 Mark Tompsett 2015-06-22 05:14:54 UTC
Comment on attachment 36022 [details] [review]
Bug 13719 - Make Koha::Objects store list of resultant objects on an as needed basis

Review of attachment 36022 [details] [review]:
-----------------------------------------------------------------

Is there some default behaviour that I'm unaware of in terms of attempting to access something past the boundary of a list?

::: Koha/Objects.pm
@@ +140,4 @@
>  sub next {
>      my ( $self ) = @_;
>  
> +    my $object = $self->_objects()->[ $self->{_iterator}++ ];

No boundary check?!
Comment 8 Mark Tompsett 2015-06-22 05:16:23 UTC
On a positive note, the test plan worked, and a "time prove -v t/db_dependent/Borrowers.t" seemed to run a little faster on default data with the patch.
Comment 9 Bernardo Gonzalez Kriegel 2015-12-18 18:36:07 UTC
Created attachment 45862 [details] [review]
[SIGNED-OFF] Bug 13719: Make Koha::Objects store list of resultant objects on an as needed basis

We can both simplify and improve the functioning of Koha::Objects by
removing out reliance on DBIC for set iteration ( first(), next(),
reset(), etc ). The problem is that DBIC destroys and refetches
results every time reset() is called. For example, take the following
code:

my $borrowers = Koha::Borrowers->search({ firstname => 'Kyle' }).
my $kyle1 = $borrowers->next();
$borrowers->reset();
my $kyle2 = $borrowers->next();

In this case, we would expect $kyle1 and $kyle2 to refer to the exact
same object in memory, but they do *not*. This is simply a limitation of
DBIx::Class.

However, by handling this issue ourselves, we not only solve the
problem, but I believe we also reduce the complexity of our code.

This is all accomplished without changing the external behavior of the
Koha::Objects module.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Borrowers.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test pass, no errors
Comment 10 Jonathan Druart 2015-12-23 09:28:16 UTC
From the doc
http://search.cpan.org/~ribasushi/DBIx-Class-0.082820/lib/DBIx/Class/ResultSet.pm#reset

"Implicitly resets the storage cursor, so a subsequent "next" will trigger another query."

It is not a limitation or a bug, but a feature.
I am not sure it's a good idea to make our methods behave differently than the DBIx::Class ones.
Comment 11 Jonathan Druart 2015-12-23 09:29:29 UTC
CCing Martin to get another feedback.
Comment 12 Martin Renvoize 2015-12-23 12:48:11 UTC
Totally agree with Jonathan here, I don't understand the point of this at all.. the idea of reset is to allow one to re-execute the query in the case that the data in the database may have changed between resultset usage.

If you want to make it more likely $kyle1 == $kyle2 then add an order_by into the search. (but of course, this won't ensure unless of course you're ordering by id and you've not changed the original object at in the database)

I've not looked into the code yet, but just on the description I'm not really sure what problem it's attempting to resolve.
Comment 13 Martin Renvoize 2015-12-23 13:00:31 UTC
OK, So, you're always calling ->all on he dbic resultset, even when you may only actually want the ->first result :(. Bad mojo, this is a big memory leak in my opinion.

Yes calling ->all is a good idea when you know you want the full set (or you know the full set will always be sufficiently small that your not worried about the memory overhead, but calling it for all requests is just asking for trouble in my opinion.

We should be encouraging thought behind these calls and good practice. Not coding around other coders deficiencies.. that's what QA is for, spotting such performance mistakes during QA time.

All in all, this gets a definitive thumbs down from me
Comment 14 Kyle M Hall 2016-01-07 16:41:34 UTC
(In reply to Martin Renvoize from comment #13)
> OK, So, you're always calling ->all on he dbic resultset, even when you may
> only actually want the ->first result :(. Bad mojo, this is a big memory
> leak in my opinion.
> 
> Yes calling ->all is a good idea when you know you want the full set (or you
> know the full set will always be sufficiently small that your not worried
> about the memory overhead, but calling it for all requests is just asking
> for trouble in my opinion.
> 
> We should be encouraging thought behind these calls and good practice. Not
> coding around other coders deficiencies.. that's what QA is for, spotting
> such performance mistakes during QA time.
> 
> All in all, this gets a definitive thumbs down from me

Good point. Let's just close this it out, it seemed like a good idea at first, but I think you are correct.