nhibernate-core icon indicating copy to clipboard operation
nhibernate-core copied to clipboard

NH-3530 - memory when using default_batch_fetch_size

Open nhibernate-bot opened this issue 7 years ago • 20 comments

Kris Desmadryl created an issue:

As since NH3+ setting the default_batch_fetch_size to something has very bad impact on memory as soon the SessionFactory is created. I'm using 3 session factories in an web app (I'm accessing 3 db's). It uses 250MB without the setting but 750MB (default_batch_fetch_size=16) with the setting. When I switch back to 2.1.2 the problem is gone. My webserver recycles because the private memory is reached!

grtz, Kris


Filip Duyck added a comment — :

In addition to what Kris said, I'd like to share some results from our dotTrace profiling session.

Performance trace can be seen at -- notice how in NHibernate 3, EntityLoader..ctor is called about 10x as much as in 2.1.2 (even though the calls to BatchingEntityLoader.CreateBatchingEntityLoader are more or less of the same order of magnitude).

We also did memory profiling which showed similar results, which you can see at . In this trace, you can see that BatchingEntityLoader.CreateBatchingEntityLoader creates many, many more objects in 3.3.3 than it did in 2.1.2.

I have looked at the code of both methods but I can't really identify a change. Our setup for NH2.1.2 and NH3.3.3 use virtually the same configuration. The problem is, as Kris mentioned, amplified by the fact that we have defaultbatch_fetchsize set to 16 which causes more looping inside the CreateBatchingEntityLoader method, but this setting is the same during 2.1.2 and 3.3.3 testing.

Can anyone from the NH team shed a light on this? Especially the memory problem is killing us in production.


Filip Duyck added a comment — :

It's been almost a year. Is anyone ever going to look at this?

nhibernate-bot avatar Oct 13 '17 00:10 nhibernate-bot

It seems like this is the regression of following: https://github.com/nhibernate/nhibernate-core/commit/a35aaa648b2cbd936a6a514357e8a538d74206be

hazzik avatar Dec 05 '18 23:12 hazzik

We wanted to start using default_batch_fetch_size, but after enabling it, our tests started to throw OutOfMememory exceptions. Investigation led me to this issue... This is the difference in memory usage in our case, we are using NH 5.2.3

with default_batch_fetch_size=20...

  • alocations image
  • memory image

without

  • allocations image
  • memory image

Unfortunately this issue prevents us from using this feature...

LeMaciek avatar Jun 10 '19 08:06 LeMaciek

You may use instead discrete batch-size settings (hbm attributes) on classes and collections requiring it, instead of enabling it for all of them through default_batch_fetch_size. This will reduce the memory impact, but it is also more work on the developer side. That is of course a work-around. This feature should not have such a memory impact.

fredericDelaporte avatar Jun 10 '19 18:06 fredericDelaporte

The memory impact is due to the BatchingEntityLoader creating many EntityLoader instances which have pre-built sql statements for fetching the entity for different batch sizes. When batch fetch size 20 is used the BatchingEntityLoader will create eleven EntityLoader instances with batch sizes: 20, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, which is the reason for the high memory usage. One way to solve the issue is to port HHH-7746 which contains an additional setting hibernate.batch_fetch_style that affect how batching of entities is done. By using BatchFetchStyle .DYNAMIC the memory usage would not be affected as the sql statements are created dynamically, which would solve the OutOfMemoryException for those that have a limited amount of memory, but it will use more cpu due to dynamic sql generation.

maca88 avatar Jun 10 '19 19:06 maca88

Related: #1904, which would be an intermediate solution, maybe insufficient for some applications (if they usually end up using most loaders during their lifetime).

fredericDelaporte avatar Jan 05 '20 17:01 fredericDelaporte

Additional details in #2427

hazzik avatar Jul 10 '20 03:07 hazzik

We ran in this issue, too. We have got round about 300 hundreds web applications using NHibernate under the hood. The usage of the batch size would help us to avoid SELECT N+1-problems etc. Thus a solution similar to Hibernate would be really great. What can I do to force the implementation?

Aldomat avatar Oct 30 '20 10:10 Aldomat

Hi All

We're a sponsor of the firebird project (we sponsor practically the complete .net driver for firebird). We migrated from LLBLGenPro to NHibernate. But, we're facing a severe issue with the problem described above. We have 50 databases on a server and hundreds of tables. Batching is set to 20 or 100 depending on table. We don't set default_batch_fetch_size, but still it requires a few GB just to start the project.

Is there somebody who can implement DYNAMIC batching on short term? We're willing to sponsor the project.

kind regards

Alexander

AlexanderMuylaert avatar Apr 09 '21 06:04 AlexanderMuylaert

Hi Alexander,

I reviewed the code and to me it is obvious that the memory won't be released. I have a fix locally which I will provide next week.

Kind regards, Carsten

Aldomat avatar Apr 09 '21 08:04 Aldomat

Hi Carsten

Any progress on this topic about dynamic sql generation as opposed of keeping all these "in ()" statements cached.

kind regards

Alexander

AlexanderMuylaert avatar Apr 27 '21 14:04 AlexanderMuylaert

Hi

Any luck on this? It is getting extremely urgent for us since it is actually freezing our servers all the time.

many thanks

Alexander

AlexanderMuylaert avatar Aug 19 '21 13:08 AlexanderMuylaert

As far as I know, no one works on this trouble currently. PR are welcome.

fredericDelaporte avatar Aug 19 '21 19:08 fredericDelaporte

We have a solution on this. In our next sprint (three weeks) we need to prove that this has the expected behaviour. The problem with the solution above is that the memory will not be freed. Hopefully we can provide a solution in first week of october.

Kind regards Carsten

cal-schleupen avatar Aug 23 '21 07:08 cal-schleupen

Hi all

How are you doing on this issue? Can I follow it up somewhere?

many thanks

Alexander

AlexanderMuylaert avatar Oct 14 '21 13:10 AlexanderMuylaert

Hi Alexander, currently a developer has reviewed the fix and veryfied that the memory won't grow. That has been successful on first stage. Now we are checking that against a process with bulk data. Than we try to upload that fix.

The fix provided in https://github.com/nhibernate/nhibernate-core/pull/1904/files was unsufficient because the memory still increases. So we needed to introduce a factory, so that the loaders will be created by a factory and the garbage collector can free memory.

The following shows the difference: image

The fix will hopefully available during the next two weeks. Sorry for the delay. We are very busy at the moment.

Kind regards Carsten

cal-schleupen avatar Nov 24 '21 10:11 cal-schleupen

I guess it would work. But it's not very nice solution to re-create loaders on each load operation. So it's good only for custom forks. The proper way to solve it described here

Is there somebody who can implement DYNAMIC batching on short term? We're willing to sponsor the project

@AlexanderMuylaert If it's still a deal I can port DYNAMIC batching. I will have time to work on it in 2 weeks and I think I would need 1 week to have it implemented (though you would still need to stick to custom fork for some time till it's reviewed by other members)

bahusoid avatar Nov 25 '21 12:11 bahusoid

I agree that the other implementation would be better. But our tests show that we can solve select N+1-problems and get better performance with that solution. Memory was also not increasing. Unfortunately, we don't have the knowledge to implement it as described in https://github.com/nhibernate/nhibernate-core/issues/1316#issuecomment-500570009 and not the time to do it. Sorry!

cal-schleupen avatar Nov 25 '21 13:11 cal-schleupen

Hi

Checking it out for you with new management.

Kind regards

Alexander

Get Outlook for Androidhttps://aka.ms/AAb9ysg


From: Roman Artiukhin @.> Sent: Thursday, November 25, 2021 1:54:15 PM To: nhibernate/nhibernate-core @.> Cc: AlexanderMuylaert @.>; Mention @.> Subject: Re: [nhibernate/nhibernate-core] NH-3530 - memory when using default_batch_fetch_size (#1316)

I guess it would work. But it's not very nice solution to re-create loaders on each load operation. So it's good only for custom forks. The proper way to solve it described herehttps://github.com/nhibernate/nhibernate-core/issues/1316#issuecomment-500570009

Is there somebody who can implement DYNAMIC batching on short term? We're willing to sponsor the project

@AlexanderMuylaerthttps://github.com/AlexanderMuylaert If it's still a deal I can port DYNAMIC batching. I will have time to work on it in 2 weeks and I think I would need 1 week to have it implemented (though you would still need to stick to custom fork for some time till it's reviewed by other members)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/nhibernate/nhibernate-core/issues/1316#issuecomment-979189430, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AK65J35XG6BP27XOJCSW2ULUNYWXPANCNFSM4D67F53A. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

AlexanderMuylaert avatar Nov 25 '21 15:11 AlexanderMuylaert

Hi

Any rough estimation on the total price? It looks okay for our mgmt, but they need a rough estimation.

Kind regards

Alexander

Get Outlook for Androidhttps://aka.ms/AAb9ysg


From: Roman Artiukhin @.> Sent: Thursday, November 25, 2021 1:54:15 PM To: nhibernate/nhibernate-core @.> Cc: AlexanderMuylaert @.>; Mention @.> Subject: Re: [nhibernate/nhibernate-core] NH-3530 - memory when using default_batch_fetch_size (#1316)

I guess it would work. But it's not very nice solution to re-create loaders on each load operation. So it's good only for custom forks. The proper way to solve it described herehttps://github.com/nhibernate/nhibernate-core/issues/1316#issuecomment-500570009

Is there somebody who can implement DYNAMIC batching on short term? We're willing to sponsor the project

@AlexanderMuylaerthttps://github.com/AlexanderMuylaert If it's still a deal I can port DYNAMIC batching. I will have time to work on it in 2 weeks and I think I would need 1 week to have it implemented (though you would still need to stick to custom fork for some time till it's reviewed by other members)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/nhibernate/nhibernate-core/issues/1316#issuecomment-979189430, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AK65J35XG6BP27XOJCSW2ULUNYWXPANCNFSM4D67F53A. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

AlexanderMuylaert avatar Nov 26 '21 16:11 AlexanderMuylaert

@AlexanderMuylaert Can you please contact me via email to discuss it further

bahusoid avatar Nov 26 '21 17:11 bahusoid