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.

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

Memory leaking with Serializing JSON on Large Objects #75314

Closed
AhmedZero opened this issue Sep 9, 2022 · 31 comments
Closed

Memory leaking with Serializing JSON on Large Objects #75314

AhmedZero opened this issue Sep 9, 2022 · 31 comments
Labels
area-System.Text.Json question Answer questions and provide assistance, not an issue with source code or documentation.

Comments

@AhmedZero
Copy link

AhmedZero commented Sep 9, 2022

now there is Memory leaks with JsonSerializer.Serialize when i convert list of objects, the memory increases from 950 MB to 2.5 GB.

var list = JsonSerializer.Serialize(MainWindowViewModel.Tabs, options);
File.WriteAllText(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "products.json"), list);
GC.Collect();

before JsonSerializer.Serialize
image
after JsonSerializer.Serialize
image

i test with .NET 7.0.0-rc.2.22451.11
my OS is Windows 11
the app is X86

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 9, 2022
@ghost
Copy link

ghost commented Sep 9, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

now there is Memory leaks with JsonSerializer.Serialize when i convert list of objects, the memory increases from 950 MB to 2.5 GB.
```C#
var list = JsonSerializer.Serialize(MainWindowViewModel.Tabs, options);
File.WriteAllText(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "products.json"), list);
GC.Collect();

before  `JsonSerializer.Serialize` 
![image](https://user-images.githubusercontent.com/26312527/189248348-3169e369-1f6d-4da2-a437-4ec5808f1cb1.png)
after  `JsonSerializer.Serialize` 
![image](https://user-images.githubusercontent.com/26312527/189248418-ed92ff4e-5ba4-4d45-9130-f2584f4e749c.png)

i test with .NET 7.0.0-rc.2.22451.11
my OS is Windows 11
the app is X86

<table>
  <tr>
    <th align="left">Author:</th>
    <td>AhmedZero</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`area-System.Text.Json`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>-</td>
  </tr>
</table>
</details>

@stephentoub
Copy link
Member

Is it actually a leak? Has the GC run? If you look at the objects still alive, where are they rooted?

My guess is these are large arrays in the ArrayPool, and there simply hasn't been enough memory pressure and/or GCs to cause it to release them all.

@AhmedZero
Copy link
Author

I don't know but I build the app again with x64 and my PC rams 16GB and the same problem.
IMG_20220909_040616.jpg

@stephentoub
Copy link
Member

Working set doesn't mean there's a leak. In a managed environment, the GC gets to decide when to decommit memory.

I'd recommend taking a memory profile:
https://docs.microsoft.com/en-us/visualstudio/profiling/memory-usage-without-debugging2?view=vs-2022
and in the resulting snapshot looking to see what objects are taking up all the space and what their "roots" are. That will tell you if there's actually a leak or not.

@eiriktsarpalis
Copy link
Member

Couple of questions:

  • Is this a regression from .NET 6?
  • Can you share a console app containing a minimal reproduction of the issue?

Thanks!

@eiriktsarpalis eiriktsarpalis added the needs-author-action An issue or pull request that requires more info or actions from the author. label Sep 9, 2022
@ghost
Copy link

ghost commented Sep 9, 2022

This issue has been marked needs-author-action and may be missing some important information.

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 9, 2022
@AhmedZero
Copy link
Author

1- No, it's .NET 7.0.0-rc.2.22451.11
2- anyway it's Avalonia, not console, and I think the problem is caused by serialization of the class inherited from ReactiveObject. and it has a list containing more than 100,000 Objects.
anyway, I'll specify which cause the memory leak in JsonSerializer.Serialize by using dotmemory.
public sealed class ProductInformations : ReactiveObject

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Sep 9, 2022
@AhmedZero
Copy link
Author

IMG_20220910_012655.jpg

@stephentoub
Copy link
Member

The title of that view is missing, but it appears to be the stack that allocated the objects. What's more interesting is the path to root that's keeping the objects alive.

@AhmedZero
Copy link
Author

Is it related to the same problem or not?
#66102

@AhmedZero
Copy link
Author

Do you need any more information???

@stephentoub
Copy link
Member

Is it related to the same problem or not?

I'm not yet convinced there's a problem.

Do you need any more information???

Yes, what I asked for above:
"in the resulting snapshot looking to see what objects are taking up all the space and what their "roots" are"
"What's more interesting is the path to root that's keeping the objects alive."

Alternatively if you have a dump of the process to share that's fine, too.

@AhmedZero
Copy link
Author

AhmedZero commented Sep 10, 2022

IMG_20220910_025841.jpg
There is anyway to serialize only specific fields only? Or I should make another class to prevent that problem?

@AhmedZero
Copy link
Author

AhmedZero commented Sep 10, 2022

I known that but I need ignore all and add attribute like [Jsonspecific] to serialize that.

@davidfowl
Copy link
Member

Maybe you should write a custom JSON converter. It looks like we don't have any memory leak here though, so the issue is resolve right?

@AhmedZero
Copy link
Author

But why duplicates when I serialize it and still alive even I don't use it or create it?

@AhmedZero
Copy link
Author

I will test serialize with Newtonsoft JSON.NET if the same case I'll use custom converter.

@AhmedZero
Copy link
Author

I use Newtonsoft JSON.NET
before Newtonsoft.Json.JsonConvert.SerializeObject
image
after Newtonsoft.Json.JsonConvert.SerializeObject
image

@AhmedZero
Copy link
Author

AhmedZero commented Sep 10, 2022

comparison before and after using System.Text.Json serialize
image

image

@davidfowl
Copy link
Member

The serializer uses pooled memory so it’s allocated and reused for future calls. As for your specific situation, I’m still not clear on what the leak is… Maybe providing a runnable sample would be the next step here.

@stephentoub
Copy link
Member

comparison before and after using System.Text.Json serialize

The serializer uses pooled memory so it’s allocated and reused for future calls

Right, this is what I was expecting we'd see.
"My guess is these are large arrays in the ArrayPool, and there simply hasn't been enough memory pressure and/or GCs to cause it to release them all."

@AhmedZero
Copy link
Author

AhmedZero commented Sep 11, 2022

why newtonsoft reverse 400 MB and system.text.json reverses 1.6 GB? After serializing
Anyway, I'll make a sample.

@AhmedZero
Copy link
Author

@davidfowl
Copy link
Member

davidfowl commented Sep 12, 2022

Most of the memory seems to be coming from the fact that you're deriving from this ReactiveObject type. I'm not sure if that's just a convenience but the entire object graph is being serialized that that's not what you want. There's no leak here.

I'm pretty sure the difference between this an JSON.NET is the fact that it respects IgnoreDataMemberAttribute which is used by ReactiveUI to ignore specific members on ReactiveObject.

@AhmedZero
Copy link
Author

1- No, it's .NET 7.0.0-rc.2.22451.11
2- anyway it's Avalonia, not console, and I think the problem is caused by serialization of the class inherited from ReactiveObject. and it has a list containing more than 100,000 Objects.
anyway, I'll specify which cause the memory leak in JsonSerializer.Serialize by using dotmemory.
public sealed class ProductInformations : ReactiveObject

I told you that.
it's problem from ReactiveObject. and I want to ignore them.

@davidfowl
Copy link
Member

I told you how to ignore them, but you don't own ReactiveObject so the attributes won't do you any good. Write a custom JSON converter and be done with it 😄.

@AhmedZero
Copy link
Author

I already write a custom JSON converter, but I hope to support it soon like JSON.NET
[JsonObject(MemberSerialization.OptIn)]

@davidfowl
Copy link
Member

Maybe there's some clever way to use JsonInclude that I'm not aware of to solve this problem.

cc @eiriktsarpalis

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Sep 12, 2022

There's not much that can be done with base types in third-party libraries using attributes. Starting with .NET 7 it should be possible to influence what properties do get serialized using contract customization, which removes the need (and pitfalls) of writing a custom converter. Here's what a simplistic variant of such a contract resolver could look like:

var options = new JsonSerializerOptions
{
    TypeInfoResolver = new DefaultJsonTypeInfoResolver()
    {
        Modifiers =
        {
            static jsonTypeInfo =>
            {
                if (typeof(ReactiveObject).IsAssignableFrom(jsonTypeInfo.Type))
                {
                    IList<JsonPropertyInfo> properties = jsonTypeInfo.Properties;
                    for (int i = 0; i < properties.Count; i++)
                    {
                        if (properties[i].PropertyType.IsGenericType && 
                            properties[i].PropertyType.GetGenericTypeDefinition() == typeof(IObservable<>))
                        {
                            jsonTypeInfo.Properties.RemoveAt(i--);
                        }
                    }
                }
            }
        }
    }
};

Using that contract resolver in your test project resulted in memory consumption dropping substantially.

@eiriktsarpalis eiriktsarpalis added question Answer questions and provide assistance, not an issue with source code or documentation. and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Sep 12, 2022
@AhmedZero
Copy link
Author

it's better than JSON.NET now.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

No branches or pull requests

4 participants