[Bug 13719] New: Store list of objects as needed
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Bug ID: 13719 Summary: Store list of objects as needed Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35985 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35985&action=edit Bug 13719 - Store list of objects as needed -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|kyle@bywatersolutions.com |gmcharlt@gmail.com Depends on| |13019 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13726 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Assignee|gmcharlt@gmail.com |kyle@bywatersolutions.com --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Kyle, what would "Store list of objects" mean? I'm sorry but it is too generic for my idiomatic limitations! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Store list of objects as |Make Koha::Objects store |needed |list of resultant objects | |on an as needed basis -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35985|0 |1 is obsolete| | --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 36022 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36022&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Nick Clemens <nick@quecheelibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@quecheelibrary.org -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #6 from M. Tompsett <mtompset@hotmail.com> --- Couldn't this be a memory hog? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 36022 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=36022 Bug 13719 - Make Koha::Objects store list of resultant objects on an as needed basis Review of attachment 36022: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=13719&attachment=36022) ----------------------------------------------------------------- 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?! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 --- Comment #8 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Zeno Tajoli <z.tajoli@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #36022|0 |1 is obsolete| | --- Comment #9 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- Created attachment 45862 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=45862&action=edit [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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> ---
From the doc http://search.cpan.org/~ribasushi/DBIx-Class-0.082820/lib/DBIx/Class/ResultS...
"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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- CCing Martin to get another feedback. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13719 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |WONTFIX Status|In Discussion |RESOLVED --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org