Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Out of Memory: During BulkSaveChanges with AutoChangeDetection disabled. #599

Open
m-hicks-OH opened this issue Aug 16, 2024 · 16 comments
Open

Out of Memory: During BulkSaveChanges with AutoChangeDetection disabled. #599

m-hicks-OH opened this issue Aug 16, 2024 · 16 comments
Assignees
@JonathanMagnan

Comments

@m-hicks-OH
Copy link

Description

Out of Memory saving many entities via BulkSaveChanges

Exception

The application container is being terminated due to memory issues when performing BulkSaveChanges.

Exception message:
Stack trace:

I moved over to EF plus because I thought BulkSaveChanges would solve my EF save issues.
However, I'm beginning to think the issue isn't the amount of data needing to be saved, rather, the change detection needed to figure out which data needs to be saved.

I've made sure all of my entities have the proper Entity.State on them prior to disabling _context.ChangeTracker.AutoDetectChanges and calling BulkSaveChanges with a batch size of 500. However, even with change detection disabled, I get a few hundered log entries of "BulkSave Message: "-- Completed at 08/16/2024 07:18:40 PM -- Result: 1 tables " or similar, before it finally runs out of memory and crashes.

Is there some fundamental I'm missing here? I'm not exactly sure why I would be starving for memory during save changes.

Further technical details

  • EF version: [EF Core v8.0.4]
  • EF Extensions version: [EFE Core v8.103.0.0]
  • Database Server version: [AWS Aurora MySql Server 8.0]
@JonathanMagnan JonathanMagnan self-assigned this Aug 17, 2024
@JonathanMagnan
Copy link
Member

Hello @m-hicks-OH ,

Indeed, the BulkSaveChanges method will not help you. The BulkSaveChanges generates his command the same way as SaveChanges does. Leading quickly to the same memory issue when you have too much entities.

To be able to save your entities without memory issue, you can use our IncludeGraph with BulkInsert/BulkUpdate/BulkDelete/BulkMerge method.

Last year, we made some major improvements that you can find here. Those methods are not only a lot faster but use way less memory SaveChanges requires (sometimes less than 20% of the memory).

Let me know if that answers your question correctly.

Best Regards,

Jon

@JonathanMagnan
Copy link
Member

Hello @m-hicks-OH ,

Since our last conversation, we haven't heard from you.

Let me know if you need more information.

Best regards.

Jon

@m-hicks-OH
Copy link
Author

Oh, sorry, I got your email, but wasn't able to see the issue on this forum, but now I can.

Thank you for replying. I'm currently giving the BulkUpdate/Insert a try with includeGraph. It's looking like the Include graph will only ever include child entities where the configution is defined ON the parent. In most of our EF entities, the one to many relationship is defined on the entity with the FK (the child). So, I started to look at using IncludeGraphOperationBuilder to see if I could specify which entities to include. I'm having a hard time finding examples of that; rather, just examples of "how" to include them.

@JonathanMagnan
Copy link
Member

Hello @m-hicks-OH ,

Indeed, we do not yet support this scenario where the FK is defined on the child, as we are not currently looking at the ChangeTracker.

We are currently developing a solution to support this scenario, but it might still take a few months if we succeed.

In the most basic scenario, you can perform a Bulk Operations by reversing the graph:

// items = OrderDetail, which has a reference to the parent Order
context.BulkInsert(items, x => x.IncludeGraph);

However, it will not work if the invoice has more indirect navigation.

Another solution is doing multiple independent bulk operations and propagating the id if needed:

context.BulkInsert(invoice);
context.BulkInsert(items);

A little bit more complex, surely, but that will be the most optimized solution.

Let me know if that answers your question.

Best Regards,

Jon

@m-hicks-OH
Copy link
Author

Okay, I've tried a few things.

I tried saving the child and using include graph, but that ended up inserting a bunch of relationships I didn't want it to insert (references that should be readonly). So, I used the optionbuilder to setup to try to mark those entities as readonly.


        public async Task BulkSaveCustomImportDataAsync(int userId, CustomImportInstance importInstance, CancellationToken cancellationToken)
        {
            _logger.LogInformation("Updating Audit fields for firm context");
            UpdateAuditFields(_firmContext, userId);

            var bulkSaveOptions = new Action<BulkOperation>(options =>
            {
                options.BatchSize = 500;
                options.IncludeGraph = true;
                options.IncludeGraphOperationBuilder = operation =>
                {
                    operation.IsReadOnly = operation switch
                    {
                        BulkOperation<CustomImportInstance> => true,
                        BulkOperation<CustomImportTemplate> => true,
                        BulkOperation<CustomImportTemplateColumn> => true,
                        BulkOperation<CustomImportInstanceRow> => true,
                        BulkOperation<CustomImportInstanceRowState> => true,
                        _ => operation.IsReadOnly
                    };
                };

                const int loggingInterval = 200;
                var currentLogIndex = 1;
                options.Log = s => ConditionalLogger(s, ref currentLogIndex);
                return;

                void ConditionalLogger(string message, ref int loggingIndex)
                {
                    if (loggingIndex < 10 || loggingIndex % loggingInterval == 0)
                    {
                        _logger.LogInformation("BulkSave Message: {Message}", message);
                    }

                    loggingIndex++;
                }
            });


            _logger.LogInformation("Saving changes for firm context");
            await _firmContext.BulkInsertAsync(importInstance.InstanceRows, bulkSaveOptions, cancellationToken);

            await _firmContext.CustomImportInstances
                .SingleUpdateAsync(importInstance, opt => opt.IncludeGraph = false, cancellationToken);
        }

This is hasty code, written just for proof of concept, and obvoiusly, the relationship is a little more than previously spoken about.

The core concept is that we have templates defining columns(types of data) that can be used to run a "Custom Import" where a user uploads a file (set of data) matching a specific template type. We create an empty instance for them (in another set of code) and set it to the state of "Validating". Then we validate every row in the import and create new customImportInstanceRows/Columns for them (holding all of their provided values in staging tables (real tables in the database). The CustomImportInstance is the "parent" record for all of those rows and columns. So, the ONLY thing we want to insert are the rows and columns, and update the Instance.

With the code listed above, it always inserts a new CustomImportTemplate and set of CustomImportTemplateColumn-- which is not desired. Now, of course, if I disable Graph inclusion, then it correctly doesn't do that, but also doesn't include each row's columns.

I'm curious why the IsReadonly=true isn't working as I'd expect.
I'd like to avoid having to call BulkInsertAsync() on the CustomImportRowColumn, because we simply don't have that entire collection in memory, we'd have to select it out of each row.

Is there some fundamental about this that I'm just missing?

@m-hicks-OH
Copy link
Author

Okay, two updates:
I just realized I was trying to readonly the "CustomImportInstanceRow" which was not correct (those should be updated)-- not sure how that didn't cause rows to not be inserted, but they were still inserted.

Second, changing the code as follows, did fix the insert issue, but I'm concerned of memory issues at scale. As, the
CustomImportRowColumn is the entity with the largest number of records to insert by a long margin. They're already in memory on the importInstance.InstanceRows objects, and this would pull them all into yet one more collection.

            await _firmContext.BulkInsertAsync(importInstance.InstanceRows, bulkSaveOptions, cancellationToken);
            await _firmContext.BulkInsertAsync(importInstance.InstanceRows.SelectMany(r => r.Columns), bulkSaveOptions, cancellationToken);

            await _firmContext.CustomImportInstances
                .SingleUpdateAsync(importInstance, opt => opt.IncludeGraph = false, cancellationToken);

@m-hicks-OH
Copy link
Author

Interesting...
I switched to BulkMergeAsync() and put the includeGraph and optionBuilder back, and it's working as intented.

It seems like BulkInsertAsync() always follows the graph, and potentially was ignoring rules in optionBuilder to ignore.

@m-hicks-OH
Copy link
Author

image
Hmm seems like it's still trying up update TemplateColumn and Template

@JonathanMagnan
Copy link
Member

Hello @m-hicks-OH ,

Unfortunately, we don't have a solution ready for your exact scenario through the IncludeGraph

However, one will eventually come. We are currently developing a solution that will allow you to more easily choose exactly what you want to save, which looks to be exactly what you want. However, it will take several weeks before we complete it.

If you want some help finding a way to save those entities correctly, you will need to provide a runnable project with only the minimum code (not the whole project) in addition to the summary of how you want it to behave. You can send it in private here: [email protected]

That will help us to make sure we 100% understand what you are trying to achieve.

Best Regards,

Jon

@m-hicks-OH
Copy link
Author

I appreciate your time, Jonathan.
I'll continue to work on this solution to see if I can leverage a combination of EF Plus and some other changes to get the results we need.

@m-hicks-OH
Copy link
Author

m-hicks-OH commented Oct 30, 2024 via email

@JonathanMagnan
Copy link
Member

Hello Michael,

If you are using EF Core, the trial will already work until the end of November. At mid-November, we will already have a version released with the trial that will work until the end of December.

So technically, you do not need to wait for the 1st to download the new package.

Let me know if that answer to your question.

Best Regards,

Jon

@m-hicks-OH
Copy link
Author

m-hicks-OH commented Oct 30, 2024 via email

@m-hicks-OH
Copy link
Author

m-hicks-OH commented Oct 30, 2024 via email

@m-hicks-OH
Copy link
Author

m-hicks-OH commented Oct 30, 2024 via email

@JonathanMagnan
Copy link
Member

Hello @m-hicks-OH ,

Indeed, if you use EF Core, the v8.103.6 trial will be enabled until the end of November. Giving you plenty of time for your setup ;)

Best Regards,

Jon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JonathanMagnan JonathanMagnan

Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@JonathanMagnan @m-hicks-OH