| chat.freenode.net #tryton log beginning Fri Oct 25 00:00:03 CEST 2013 | ||
| 2013-10-25 12:16 <giedrius> recently i've noticed a big performance problem on tryton. When I try to save a record with a single field modified (ex. field type == integer), Tryton's ORM makes validation of all fields available on the record. So then the record has many relational fields, it becomes very slow | ||
| 2013-10-25 12:19 <giedrius> to be more specific, the problematic part is in ModelStorage._validation, where it validates domains of relational fields | ||
| 2013-10-25 12:19 <cedk> giedrius: this validation exists since 1.0 | ||
| 2013-10-25 12:19 <giedrius> i have an idea how to improve it | ||
| 2013-10-25 12:20 <giedrius> basically we need to check if modified field has any impact in validation of relational field. If no, then no need to make any query to db | ||
| 2013-10-25 12:21 <cedk> giedrius: except the target could also have changed | ||
| 2013-10-25 12:23 <giedrius> but it doesn't matter, because these fields are not touched | ||
| 2013-10-25 12:23 <cedk> giedrius: it matters | ||
| 2013-10-25 12:23 <cedk> giedrius: it is about data integrity | ||
| 2013-10-25 12:23 <giedrius> i'll tell you my case | ||
| 2013-10-25 12:26 <giedrius> i need to update unit_price for >200 StockMoves. Now if i save every record it takes 40 secs (single record ~0,2 sec.). This is not acceptable for the user. | ||
| 2013-10-25 12:27 <giedrius> cedk: the target must be responsible for integrity | ||
| 2013-10-25 12:29 <cedk> giedrius: no because the link depends on the target value | ||
| 2013-10-25 12:30 <giedrius> cedk: i mean, if changing target makes impact on other objects, it must make some kind of validation, because if it doesn't, then these object will be invalid till someone change them (until _validate is triggered) | ||
| 2013-10-25 12:30 <cedk> giedrius: your problem will be solved with the write multi | ||
| 2013-10-25 12:31 <cedk> giedrius: yes it is not prefect the other validation way should be managed by the target | ||
| 2013-10-25 12:31 <giedrius> cedk: and changing things like single integer field, it should not matter on this | ||
| 2013-10-25 12:31 <cedk> giedrius: it could matter | ||
| 2013-10-25 12:32 <giedrius> cedk: yes it could, only i case then the changed field is used in relation field domain | ||
| 2013-10-25 12:32 <giedrius> only in case* | ||
| 2013-10-25 12:33 <giedrius> so my suggestion is to check if changed field has any impact on record's relation fields and if so, only then query the db | ||
| 2013-10-25 12:34 <cedk> giedrius: for me, it is the wrong answer to your performance issue | ||
| 2013-10-25 12:36 <giedrius> cedk: it would be answer not only for this issue, but also would increase performance of all other modules | ||
| 2013-10-25 12:37 <giedrius> cedk: also i think this kind of validation is wrong, because user may not understand why he cannot save the record. He cannot know that things changed in other objects | ||
| 2013-10-25 12:39 <giedrius> cedk: ex. user change date of invoice, but he cannot save the record and gets the error: invalid invoice lines. He cannot understand what happens, because he only changed the date. | ||
| 2013-10-25 12:39 <cedk> giedrius: yes but at least the bug is discovered otherwise it could never been | ||
| 2013-10-25 12:40 <cedk> giedrius: and you will get the same behavior with your optimisation | ||
| 2013-10-25 12:41 <giedrius> cedk: no, because if the date field is not used in domain or state of invoice lines, then invoice lines are not validated at all. | ||
| 2013-10-25 12:42 <cedk> giedrius: yes but if it is, you still have an error message that user doesn't understand | ||
| 2013-10-25 12:42 <giedrius> cedk: hmm, i don't understand what you mean. What kind of error? | ||
| 2013-10-25 12:43 <cedk> giedrius: change date -> raise line is not valid | ||
| 2013-10-25 12:44 <cedk> giedrius: for the usee pov, it is the same strange error | ||
| 2013-10-25 12:45 <cedk> giedrius: by the way, have you metrics? | ||
| 2013-10-25 12:45 <giedrius> cedk: do you mean change date -> raise line is not valid, then the date is in the domain of invoice line? | ||
| 2013-10-25 12:45 <cedk> giedrius: yes | ||
| 2013-10-25 12:46 <cedk> giedrius: indeed, I will could agree on such optimisation if it is the only possible one | ||
| 2013-10-25 12:46 <cedk> giedrius: so I think some metrics are needed here | ||
| 2013-10-25 12:46 <cedk> giedrius: like with target search takes time with which domain etc. | ||
| 2013-10-25 12:47 <giedrius> cedk: yeah, it such case no workaround for this - user must be warned | ||
| 2013-10-25 12:47 <giedrius> in* | ||
| 2013-10-25 12:47 <cedk> giedrius: because I just see 5 Many2One on stock.move so you are just removing 5 queries out of I guess ~200 | ||
| 2013-10-25 12:49 <giedrius> cedk: saving StockMove does not make up to 200 queries, its only about 5-6 | ||
| 2013-10-25 12:49 <cedk> giedrius: needs metrics | ||
| 2013-10-25 12:50 <giedrius> cedk: i'm pretty sure that in some cases improvements will be >100x (ex. in mine) | ||
| 2013-10-25 12:50 <cedk> giedrius: I think you are under-estimate | ||
| 2013-10-25 12:50 <giedrius> cedk: ok, i'll try to implement this | ||
| 2013-10-25 12:51 <giedrius> cedk: could be, but why not try | ||
| 2013-10-25 12:52 <cedk> giedrius: that what I say we need metrics | ||
| 2013-10-25 12:54 <giedrius> cedk: sure, not problem for this. I just was wondering if this would not make any problem with functionality | ||
| 2013-10-25 12:55 <cedk> giedrius: hard to say but we have tests for that | ||
| 2013-10-25 13:09 <cedk> giedrius: by the way, I think you change should do this: | ||
| 2013-10-25 13:09 <cedk> - change: _validate(cls, records, fields=None) | ||
| 2013-10-25 13:10 <cedk> - on the loop over fields, add test field_name in fields or any field.depends in fields | ||
| 2013-10-25 13:18 <giedrius> cedk: i was already started thinking how to parse the domain to get these fields. Cool, field.depends makes this easy :) | ||
| 2013-10-25 13:20 <cedk> giedrius: of course this will depend on the good definition but there is a default unittest to check | ||
| 2013-10-25 15:36 -!- ykarmouta(~ykarmouta@LNeuilly-152-23-15-185.w193-252.abo.wanadoo.fr) has left #tryton | ||
| 2013-10-25 19:00 -!- vcardon(~vcardon@LNeuilly-152-23-15-185.w193-252.abo.wanadoo.fr) has left #tryton |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!