ModelStorage.create() should accept a list of dictionaries as parameter Issue2358 (issue266002)
albert....@gmail.com
Please review this at http://codereview.tryton.org/266002/
Affected files:
M trytond/model/modelsql.py
M trytond/model/modelstorage.py
M trytond/tests/test_tryton.py
Albert Cervera i Areny
A Diumenge, 4 de mar� de 2012 03:49:24, albert....@gmail.com va escriure:
I recently updated this patch to tip. It'd be great if somebody could review it. Most of the code does not require in-depth knowledge of trytond core.
--
Albert Cervera i Areny
Tel: +34 93 553 18 03
Cédric Krier
> A Diumenge, 4 de març de 2012 03:49:24, albert....@gmail.com va escriure:
> >
> >
> >
> > Please review this at http://codereview.tryton.org/266002/
> >
> > Affected files:
> > M trytond/model/modelsql.py
> > M trytond/model/modelstorage.py
> > M trytond/tests/test_tryton.py
>
> I recently updated this patch to tip. It'd be great if somebody could review
> it. Most of the code does not require in-depth knowledge of trytond core.
backward compatible changes.
So I'm wondering if we should not do it for this patch.
Some options are possible:
- allow create to work with a list or a dict. I don't like this
solution because we will have the same issue as for
write/delete/browse
- rename create into create_list and add a method create that
convert the dict into a list.
Or we decide that the change worth the non-backward compatible API
change.
--
Cédric Krier
B2CK SPRL
Rue de Rotterdam, 4
4000 Liège
Belgium
Tel: +32 472 54 46 59
Email/Jabber: cedric...@b2ck.com
Website: http://www.b2ck.com/
Albert Cervera i Areny
A Dissabte, 17 de novembre de 2012 23:29:08, C�dric Krier va escriure:
> On 17/11/12 20:48 +0100, Albert Cervera i Areny wrote:
> > A Diumenge, 4 de mar� de 2012 03:49:24, albert....@gmail.com va escriure:
> > > Reviewers: ,
> > >
> > >
> > >
> > > Please review this at http://codereview.tryton.org/266002/
> > >
> > > Affected files:
> > > M trytond/model/modelsql.py
> > > M trytond/model/modelstorage.py
> > > M trytond/tests/test_tryton.py
> >
> > I recently updated this patch to tip. It'd be great if somebody could
> > review it. Most of the code does not require in-depth knowledge of
> > trytond core.
>
> We talked at the TUL that we should stabilize the API or at least make
> backward compatible changes.
> So I'm wondering if we should not do it for this patch.
> Some options are possible:
>
> - allow create to work with a list or a dict. I don't like this
> solution because we will have the same issue as for
> write/delete/browse
>
> - rename create into create_list and add a method create that
> convert the dict into a list.
>
> Or we decide that the change worth the non-backward compatible API
> change.
I know API changes are a pain but I really think this one is worth it for performance reasons. Let me show you the results of creating a thousand parties and a thousand products (with their templates) in my machine with and without the patch (average of three runs) with sqlite:
Parties:
tip = 4.57 (42% slower than patched version)
patched = 3.22 (70% of tip)
Products:
tip = 32.18 (45% slower than patched version)
patched = 22.25 (69% of tip)
I also tried creating records one by one with the patched version and no performance loss was detected compared with tip.
Cédric Krier
> A Dissabte, 17 de novembre de 2012 23:29:08, Cédric Krier va escriure:
> > > > Reviewers: ,
> > > >
> > > >
> > > >
> > > > Please review this at http://codereview.tryton.org/266002/
> > > >
> > > > Affected files:
> > > > M trytond/model/modelsql.py
> > > > M trytond/model/modelstorage.py
> > > > M trytond/tests/test_tryton.py
> > >
> > > I recently updated this patch to tip. It'd be great if somebody could
> > > review it. Most of the code does not require in-depth knowledge of
> > > trytond core.
> >
> > We talked at the TUL that we should stabilize the API or at least make
> > backward compatible changes.
> > So I'm wondering if we should not do it for this patch.
> > Some options are possible:
> >
> > - allow create to work with a list or a dict. I don't like this
> > solution because we will have the same issue as for
> > write/delete/browse
> >
> > - rename create into create_list and add a method create that
> > convert the dict into a list.
> >
> > Or we decide that the change worth the non-backward compatible API
> > change.
>
> I know API changes are a pain but I really think this one is worth it for
> performance reasons.
community about it.
Also this change will fix: https://bugs.tryton.org/issue1892
Maximilian Holtzberg
> On 18/11/12 00:34 +0100, Albert Cervera i Areny wrote:
>> A Dissabte, 17 de novembre de 2012 23:29:08, Cédric Krier va escriure:
>>
>> performance reasons.
>
> I agree about the benefit. But I would like to know the feeling of the
> community about it.
> Also this change will fix: https://bugs.tryton.org/issue1892
>
>
Max
Raimon Esteve
> solution because we will have the same issue as for
> write/delete/browse
don't like if it's similar write/delete/browse.
Test about time parties and products from Albert is a example about
get more speed to migration data or ecommerce (magento, paypal,...)
import values, for example, because they need a lot data creation.
Raimon
Cédric Krier
> 2012/11/17 Cédric Krier <cedric...@b2ck.com>:
> > - allow create to work with a list or a dict. I don't like this
> > solution because we will have the same issue as for
> > write/delete/browse
>
> Yes. create allow a dict or list[dict]. But I don't understand why
> don't like if it's similar write/delete/browse.
only support list and no more a single id.
Jordi Esteve
>> On 18/11/12 00:34 +0100, Albert Cervera i Areny wrote:
>>> I know API changes are a pain but I really think this one is worth
>>> it for
>>> performance reasons.
>>
>> I agree about the benefit. But I would like to know the feeling of the
>> community about it.
>> Also this change will fix: https://bugs.tryton.org/issue1892
>>
>>
>
> I would appreciate this change. I had similar performance issues .
>
that Albert has reported is important.
Jordi
--
Jordi Esteve
Consultor Zikzakmedia SL
jes...@zikzakmedia.com
M�bil 679 170 693
Zikzakmedia SL
Dr. Fleming, 28, baixos
08720 Vilafranca del Pened�s
Tel 93 890 2108
Sergi Almacellas Abellana
I agree about the benefit. But I would like to know the feeling of the
community about it.
Also this change will fix: https://bugs.tryton.org/issue1892
I would appreciate this change. I had similar performance issues .
Just my 2 cents!
Mathias Behrle
dictionaries as parameter Issue2358 (issue266002)" (Sun, 18 Nov 2012 12:53:58
+0100):
> I agree about the benefit. But I would like to know the feeling of the
> community about it.
effort.
--
Mathias Behrle
MBSolutions
Gilgenmatten 10 A
D-79114 Freiburg
Tel: +49(761)471023
Fax: +49(761)4770816
http://m9s.biz
UStIdNr: DE 142009020
PGP/GnuPG key availabable from any keyserver, ID: 0x8405BBF6
Albert Cervera i Areny
A Diumenge, 18 de novembre de 2012 00:34:17, Albert Cervera i Areny va escriure:
I did some more testing and I wanted to share my findings:
The party test I did created records like this:
{
'active': True,
'categories': [],
'lang': False,
'name': str(x),
'code': str(x),
'addresses': [],
}
Addresses was important to set because otherwise party's default value would create a new address and I wanted to check performance creating a single record. Something similar happened with lang in which case it's default value checks for party.configuration singleton.
And after cedk's comments that the bottleneck was not in the INSERT operation, I decided to try to find out if that was actually the case. I realized there was something strange in my results until I found out that actually INSERT was not taking all that much time but still there was a huge difference between the time spent in the create operation between sqlite and PostgreSQL backends.
I found out that the problem was in the defaults_get() operation because I had not provided the 'code_readonly' field. The field is of type Function so the first issue is that I think there's no need for the defaults_get() operation to return this value in the case of a create().
Even if the code_readonly was read the performance difference between providing this value and not doing so in PostgreSQL was still huge. Create took 14.5 seconds if code_readonly was not provided while it took only 0.5 seconds if code_readoly = False was provided!!! That is it takes only 0.5 seconds to create 1000 records which looks pretty good to me. The question is why the cache system didn't work for the first case
'code_readonly' default function simply reads information from party.configuration but why such a huge difference? Why did the cache not work there? The reason is simple: when a singleton has no record, its read operation executes a default_get() which in most cases means reading all its fields which tend to be Property's and that operation is relatively slow. Enough to make a 0.5s create() become 14.5s!
I think we may be hit by this problem in other places.
The question about wheather the more complex implementation that executes a single insert for all records is worth it or not may be answered by the following numbers.
With PostgreSQL backend:
Creating a 1000 records at once:
Multiple inserts: 1.45 seconds (77% speedup over current implementation)
Single insert: 0.51 seconds (65% speedup over multiple inserts, 92% speedup over current implementation)
Creating a 1000 records one by one (1000 create calls):
Multiple inserts: 6.29 seconds
Single insert: 6.38 seconds (1.4% slowdown over multiple inserts, 1.7% slowdown over current implementation)
Current implementation: 6.27 seconds
With SQLite backend:
Creating a 1000 records at once:
Simple: 0.45 seconds (73% speedup over current implementation)
Complex: 0.42 seconds (6% speedup over multiple inserts, 74% speedup over current implementation)
Creating a 1000 records one by one (1000 create calls):
Simple: 1.65 seconds
Complex: 1.66 seconds (0.6% slowdown over multiple inserts, 1.2% slowdown over current implementation)
Current implementation: 1.64 seconds
Some conclusions:
- With PostgreSQL, speedup is even higher than my first numbers if no strange default_get() issues are involved.
- With PostgreSQL, even if the main bottle neck with current implementation is addressed by the multiple inserts (simpler) implementation, there's still room for improvement using a single insert (92% instead of 77% speedup).
- We should find a solution for singletons when there's no record so we can still use the cache.
Cédric Krier
> Some conclusions:
> - With PostgreSQL, speedup is even higher than my first numbers if no strange
> default_get() issues are involved.
> - With PostgreSQL, even if the main bottle neck with current implementation is
> addressed by the multiple inserts (simpler) implementation, there's still room
> for improvement using a single insert (92% instead of 77% speedup).
will be more complex to handle my concern about length of query.
> - We should find a solution for singletons when there's no record so we can
> still use the cache.
is a correct solution. I think it is better to say, you have to create
the singleton. Also I think it is the Property field that are slow and
they should be replace by explicit Function field using a new table with
real foreign key.
Teagarden
> It could be possible to cache the default_get method but I'm not sure it
> is a correct solution. I think it is better to say, you have to create
> the singleton. Also I think it is the Property field that are slow and
> they should be replace by explicit Function field using a new table with
> real foreign key.
+1 for replacing configuration singletons with function fields and an intersection table with fk to company and the related model.
Thanks
Sharoon Thomas
Albert Cervera i Areny
A Diumenge, 2 de desembre de 2012 17:16:49, Teagarden va escriure:
Yes, that was already agreed and I think it even was on the roadmap. Also the advantage is that although it is an API change we can leave fields.Property as deprecated for one or two releases, if some people want to avoid the change.
--
Albert Cervera i Areny
A Diumenge, 2 de desembre de 2012 17:16:49, Teagarden va escriure:
> On Dec 2, 2012, at 7:35 PM, C�dric Krier <cedric...@b2ck.com> wrote:
> > It could be possible to cache the default_get method but I'm not sure it
> > is a correct solution. I think it is better to say, you have to create
> > the singleton. Also I think it is the Property field that are slow and
> > they should be replace by explicit Function field using a new table with
> > real foreign key.
>
> Property fields are probably one of the slowest components and the
> bottleneck appears to be the entity attribute value model on ir.property
Further testing, now with product.product, shows that translatable fields also impose a huge overhead. Writing a thousand 1000 products takes as much as 60 seconds with single insert and 112 seconds with multiple inserts implementations. Part of the problem is the inheritance but that almost only affects the multiple inserts implementation. In the single insert version ~57 seconds are spent adding the translations.
I have created a new page [1] to collect tips and possible improvements in terms of performance.
Sharoon Thomas
A Diumenge, 2 de desembre de 2012 17:16:49, Teagarden va escriure:
> On Dec 2, 2012, at 7:35 PM, Cédric Krier <cedric...@b2ck.com> wrote:> > It could be possible to cache the default_get method but I'm not sure it> > is a correct solution. I think it is better to say, you have to create> > the singleton. Also I think it is the Property field that are slow and> > they should be replace by explicit Function field using a new table with> > real foreign key.>> Property fields are probably one of the slowest components and the> bottleneck appears to be the entity attribute value model on ir.property
Further testing, now with product.product, shows that translatable fields also impose a huge overhead. Writing a thousand 1000 products takes as much as 60 seconds with single insert and 112 seconds with multiple inserts implementations. Part of the problem is the inheritance but that almost only affects the multiple inserts implementation. In the single insert version ~57 seconds are spent adding the translations.
I have created a new page [1] to collect tips and possible improvements in terms of performance.
Sharoon Thomas
A Diumenge, 2 de desembre de 2012 17:16:49, Teagarden va escriure:
> On Dec 2, 2012, at 7:35 PM, Cédric Krier <cedric...@b2ck.com> wrote:> > It could be possible to cache the default_get method but I'm not sure it> > is a correct solution. I think it is better to say, you have to create> > the singleton. Also I think it is the Property field that are slow and> > they should be replace by explicit Function field using a new table with> > real foreign key.>> Property fields are probably one of the slowest components and the> bottleneck appears to be the entity attribute value model on ir.property
Further testing, now with product.product, shows that translatable fields also impose a huge overhead. Writing a thousand 1000 products takes as much as 60 seconds with single insert and 112 seconds with multiple inserts implementations. Part of the problem is the inheritance but that almost only affects the multiple inserts implementation. In the single insert version ~57 seconds are spent adding the translations.
I have created a new page [1] to collect tips and possible improvements in terms of performance.
Albert Cervera i Areny
A Dilluns, 3 de desembre de 2012 07:45:45, Sharoon Thomas va escriure:
> On Dec 3, 2012, at 5:06 AM, Albert Cervera i Areny <alb...@nan-tic.com> wrote:
> > A Diumenge, 2 de desembre de 2012 17:16:49, Teagarden va escriure:
> > > On Dec 2, 2012, at 7:35 PM, C�dric Krier <cedric...@b2ck.com> wrote:
> > > > It could be possible to cache the default_get method but I'm not sure
> > > > it is a correct solution. I think it is better to say, you have to
> > > > create the singleton. Also I think it is the Property field that are
> > > > slow and they should be replace by explicit Function field using a
> > > > new table with real foreign key.
> > >
> > > Property fields are probably one of the slowest components and the
> > > bottleneck appears to be the entity attribute value model on
> > > ir.property
> >
> > Further testing, now with product.product, shows that translatable fields
> > also impose a huge overhead. Writing a thousand 1000 products takes as
> > much as 60 seconds with single insert and 112 seconds with multiple
> > inserts implementations. Part of the problem is the inheritance but that
> > almost only affects the multiple inserts implementation. In the single
> > insert version ~57 seconds are spent adding the translations.
>
> Could it be the index [1] ?
My mistake. 57 seconds is spent creating translations but also writing to properties (list_price, cost_price_method & cost_price are properties).
Here are some numbers using sqlite and multiple query implementation:
Creating a 1000 records at once: 28s
Using create([]) instead of ir.translation set_ids(): 26s
Total amount of time spent setting properties: 10s
Albert Cervera i Areny
A Diumenge, 4 de mar� de 2012 03:49:24, albert....@gmail.com va escriure:
> Reviewers: ,
I have a couple of questions that I'd like to ask before adding the changes to this patch:
- In the sale module, the code for creating an invoice uses the ActiveRecord pattern and ends up with a list of SaleLine instances. Current code uses a loop over those objects and calls .save() on each of them. This is can be improved with the new create() but I thought that the following would work and it didn't:
invoice = self._get_invoice_sale(invoice_type)
+ invoice.lines = invoice_lines
invoice.save()
I thought this would be similar to the non-ActiveRecord pattern and would be equivalent to [('create', invoice_line_values)] but it is not. So what I did is the following (using ._save_values property):
+ to_create = []
for line in self.lines:
if line.id not in invoice_lines:
continue
for invoice_line in invoice_lines[line.id]:
invoice_line.invoice = invoice.id
- invoice_line.save()
- SaleLine.write([line], {
- 'invoice_lines': [('add', [invoice_line.id])],
- })
+ to_create.append(invoice_line._save_values)
+ if to_create:
+ InvoiceLine.create(to_create)
Is that Ok?
Cédric Krier
> A Diumenge, 4 de març de 2012 03:49:24, albert....@gmail.com va escriure:
> >
> >
> >
> > Please review this at http://codereview.tryton.org/266002/
> >
> > Affected files:
> > M trytond/model/modelsql.py
> > M trytond/model/modelstorage.py
> > M trytond/tests/test_tryton.py
>
> I have a couple of questions that I'd like to ask before adding the changes to
> this patch:
>
> - In the sale module, the code for creating an invoice uses the ActiveRecord
> pattern and ends up with a list of SaleLine instances. Current code uses a
> loop over those objects and calls .save() on each of them. This is can be
> improved with the new create() but I thought that the following would work and
> it didn't:
>
> invoice = self._get_invoice_sale(invoice_type)
> + invoice.lines = invoice_lines
> invoice.save()
>
> I thought this would be similar to the non-ActiveRecord pattern and would be
> equivalent to [('create', invoice_line_values)] but it is not.
this?
But of course in your patch the code should be improved to group the
create.
> So what I did
> is the following (using ._save_values property):
>
> + to_create = []
> for line in self.lines:
> if line.id not in invoice_lines:
> continue
> for invoice_line in invoice_lines[line.id]:
> invoice_line.invoice = invoice.id
> - invoice_line.save()
> - SaleLine.write([line], {
> - 'invoice_lines': [('add', [invoice_line.id])],
> - })
> + to_create.append(invoice_line._save_values)
> + if to_create:
> + InvoiceLine.create(to_create)
>
>
> Is that Ok?
Albert Cervera i Areny
A Dilluns, 24 de desembre de 2012 01:41:43, C�dric Krier va escriure:
> On 24/12/12 01:15 +0100, Albert Cervera i Areny wrote:
> > A Diumenge, 4 de mar� de 2012 03:49:24, albert....@gmail.com va escriure:
> > > Reviewers: ,
> > >
> > >
> > >
> > > Please review this at http://codereview.tryton.org/266002/
> > >
> > > Affected files:
> > > M trytond/model/modelsql.py
> > > M trytond/model/modelstorage.py
> > > M trytond/tests/test_tryton.py
> >
> > I have a couple of questions that I'd like to ask before adding the
> > changes to this patch:
> >
> > - In the sale module, the code for creating an invoice uses the
> > ActiveRecord pattern and ends up with a list of SaleLine instances.
> > Current code uses a loop over those objects and calls .save() on each of
> > them. This is can be improved with the new create() but I thought that
> > the following would work and
> >
> > it didn't:
> > invoice = self._get_invoice_sale(invoice_type)
> >
> > + invoice.lines = invoice_lines
> >
> > invoice.save()
> >
> > I thought this would be similar to the non-ActiveRecord pattern and would
> > be equivalent to [('create', invoice_line_values)] but it is not.
>
> When reading the code, for me it is equivalent. How do you conclude
> this?
Because I get the following error when trying to process a sale from the client.
You try to read records that don't exist anymore!
(Document type: account.invoice.line)
Any idea on why this could happen?
> But of course in your patch the code should be improved to group the
> create.
If the previous code worked I could avoid the loop and thus it would be already grouped.
> > So what I did
> > is the following (using ._save_values property):
> >
> > + to_create = []
> >
> > for line in self.lines:
> > if line.id not in invoice_lines:
> > continue
> >
> > for invoice_line in invoice_lines[line.id]:
> > invoice_line.invoice = invoice.id
> >
> > - invoice_line.save()
> > - SaleLine.write([line], {
> > - 'invoice_lines': [('add', [invoice_line.id])],
> > - })
> > + to_create.append(invoice_line._save_values)
> > + if to_create:
> > + InvoiceLine.create(to_create)
> >
> >
> > Is that Ok?
>
> Not for me, _save_values should not be used as it is not part of the API.
--
Albert Cervera i Areny
Cédric Krier
> A Dilluns, 24 de desembre de 2012 01:41:43, Cédric Krier va escriure:
Albert Cervera i Areny
A Dijous, 27 de desembre de 2012 23:51:37, C�dric Krier va escriure:
> On 24/12/12 11:16 +0100, C�dric Krier wrote:
> > On 24/12/12 02:19 +0100, Albert Cervera i Areny wrote:
> > > A Dilluns, 24 de desembre de 2012 01:41:43, C�dric Krier va escriure:
> > > > On 24/12/12 01:15 +0100, Albert Cervera i Areny wrote:
> > > > > A Diumenge, 4 de mar� de 2012 03:49:24, albert....@gmail.com va
Yes, that was my mistake, sorry for the noise. The framework works as expected. I'm working on an updated patch which I will hopefully upload this weekend.
Oscar Alvarez
Happy new year, friends.
Atentamente / Best Regards,
Oscar Andres Alvarez Montero
CEO
Presik Technologies
www.presik.com
Bucaramanga
" now | future "
Albert Cervera i Areny
A Divendres, 28 de desembre de 2012 16:46:58, Oscar Alvarez va escriure:
> El 27/12/12 17:51, C�dric Krier escribi�:
> > On 24/12/12 11:16 +0100, C�dric Krier wrote:
> >> On 24/12/12 02:19 +0100, Albert Cervera i Areny wrote:
> >>> A Dilluns, 24 de desembre de 2012 01:41:43, C�dric Krier va escriure:
> >>>> On 24/12/12 01:15 +0100, Albert Cervera i Areny wrote:
> >>>>> A Diumenge, 4 de mar� de 2012 03:49:24, albert....@gmail.com va
I know it is. The tests I'm doing include sales with 6000 lines because we currently have a customer with those numbers. The good news is once the create with list patch will be ready you will be able to process (create shipment and invoice) with "just" 23 minutes (in a normal machine). Before the patch that simply didn't work (more than 20 hours).
Also, duplicating a sale order with 200 lines passes from 7 seconds to 2, which is not bad either, specially considering the number of checks Tryton does.