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

Using Reference.OneToOne to maintain the FK field in the model with Fluent mapping causes cycles and memory leak #637

Open
ilya-at-prettyneat opened this issue Jun 3, 2021 · 9 comments
Open

Using Reference.OneToOne to maintain the FK field in the model with Fluent mapping causes cycles and memory leak #637

ilya-at-prettyneat opened this issue Jun 3, 2021 · 9 comments
Labels
investigate

Comments

@ilya-at-prettyneat
Copy link

ilya-at-prettyneat commented Jun 3, 2021 •
edited
Loading

Greetings.

Given these models:

public class OwnedThingie
{
	public long Id {get;set;}
	public long OwnerId {get;set;}
	public UserMan Owner {get;set;}
}

public class UserMan
{
	public long Id {get;set;}
	public string Name {get;set;}
	public List<OwnedThingie> Thingies {get;set;}
}

and these mappings:

For<OwnedThingie>()
	.TableName("DbThingies")
	.PrimaryKey(o => o.Id, true)
	.Columns
	(
		c => 
		{
			c.Column(o => o.Id);
			c.Column(o => o.OwnerId);
			c.Column(o => o.Owner).WithName("OwnerId").Reference( um => um.Id, ReferenceType.OneToOne )
		},
		true
	);

For<UserMan>()
	.TableName("DbUsers")
	.PrimaryKey(u => u.Id, true)
	.Columns
	(
		c => 
		{
			c.Column(u => u.Id);
			c.Column(u => u.Name);
			c.Many(u => u.Thingies).Reference(o => o.OwnerId)
		},
                true
	);

This will create a gigantic memory leak as it will not recognize the owner <-> thingies as a looping relationship.

Our objective
We need to have both the column OwnerId and the property Owner on the data model.
OneToOne used to be the way to go, but unfortunately if you then want to establish proper relationships it will eat all of your memory - so much that the Heap display will mark a bunch of places as [Cycled].
(our actual app model has 17 tables mapped and spikes around 3GB of usage just for the NPOCO-powered service).

Proposed Solutions

  • Make a ForeignObjectOnly enum option which will leave to the coder the ability to declare the FK fields and throw otherwise
  • Make OneToOne recognize looping relationships with Many
  • ...perhaps there is a simple solution we are missing?

Tried:

  • Conditionally stopping it from including too many nested layers in the map. Failed.
  • Using .Result and .Computed to overcome the limitation on same-field-name. Failed.
  • Creating a different model that contained generic objects instead of actual data objects to stop the join propagation. Failed.
  • Switching everything to Foreign and pretending the FK properties are still there. Failed.

Any suggestions are extremely welcome.

Using NPoco latest, SQL Server 2019, .NET Core currently at 3.1.latest
@schotime
Copy link
Owner

schotime commented Jun 4, 2021

Yeh, I really should've never done any of this stuff. I've never once used it and all it causes is issues. :(

@ilya-at-prettyneat
Copy link
Author

ilya-at-prettyneat commented Jun 4, 2021 via email

@schotime
Copy link
Owner

schotime commented Jun 4, 2021

Yeah, to solve it we're going to have to have a recursive limiter in there, which i do for nested complex objects, but it mustn't cover many ones.

@ilya-at-prettyneat
Copy link
Author

ilya-at-prettyneat commented Jun 4, 2021 via email

@ilya-at-prettyneat
Copy link
Author

ilya-at-prettyneat commented Jun 4, 2021 via email

@ilya-at-prettyneat
Copy link
Author

ilya-at-prettyneat commented Jun 4, 2021 •
edited
Loading

In the end we identified the issue:

The loop protection was working correctly, but our issue was that it was generating gigantic trees due to our data structure referencing the same field(s) from many points.

As an example:

public class OwnedThingie
{
	public long Id {get;set;}
	public long OwnerId {get;set;}
	public UserMan Owner {get;set;}
	public List<SubOwnedThingie> SubThingies {get;set;}
}

public class UserMan
{
	public long Id {get;set;}
	public string Name {get;set;}
	public List<OwnedThingie> Thingies {get;set;}
}

public class SubOwnedThingie
{
	public long Id {get;set;}
	public long OwnerId {get;set;}
	public UserMan Owner {get;set;
	public long ThingieId {get;set;}
	public OwnedThingie Thingie {get;set;}
}

In this case, you already generate lots of paths due to SubOwnedThingie having a reference to UserMan.
Multiply that by 7-8 layers and you get the idea :)

Our solution was to limit how far the system would scan from any given column and you can see it in the pull request:
#638

@schotime
Copy link
Owner

schotime commented Jun 4, 2021

Well thats good news. Thx. Couple of questions.

  1. Should we have the limit set to 2 by default or something and you have to change it if you want to. I feel like thats better than letting it kill your memory?
  2. Can you add some tests to your PR?

@ilya-at-prettyneat
Copy link
Author

Apologies for the lack of comments, long weekend over here (mt-MT)

  1. I would avoid setting any defaults, as the killing or not of the memory is entirely related to how much the database is relational and, also, in which ways. A database which graphs closer to a star and less to a tree or a direct vertical could/would have a lesser impact on complexity parity. Additionally, there's likely a ton of configurations out there that would kind of mind being broken by a sudden change in the defaults :)

  2. Sure thing, however can't do it straight away, but I should be allocated some time for that in the near future.

@schotime
Copy link
Owner

schotime commented Jun 8, 2021

No problem.
Sounds good. Seeing as you are the only one reporting at the moment, I'll wait for those tests before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
investigate
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@schotime @ilya-at-prettyneat