Skip to content

Conversation

@simonthum
Copy link

This cuts about 20% of parsing cost and probably more in final object graph size, depending on input data.

In terms of the LoadLarge benchmark:

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
LoadLarge before 136.9 ms 3.11 ms 9.03 ms 7500.0000 3500.0000 1500.0000 39.65 MB
LoadLarge after 114.4 ms 3.70 ms 10.67 ms 6000.0000 2000.0000 1000.0000 39.65 MB

This is just a simple string.Intern call. It might make sense to emplyor pass a pool, like a XmlNameTable does with XML, to save on scalar objects not just strings.

@simonthum simonthum force-pushed the perf/interning branch 3 times, most recently from d0bbcc1 to 6d701be Compare May 14, 2025 12:40
This cuts about 20% of parsing time and probably more in final
object graph size, depending on input data of course.

In terms of the LoadLarge benchmark:

| Method           | Mean     | Error   | StdDev   | Gen0      | Gen1      | Gen2      | Allocated |
|-----------------:|---------:|--------:|---------:|----------:|----------:|----------:|----------:|
| LoadLarge before | 136.9 ms | 3.11 ms | 9.03 ms  | 7500.0000 | 3500.0000 | 1500.0000 |  39.65 MB |
| LoadLarge after  | 114.4 ms | 3.70 ms | 10.67 ms | 6000.0000 | 2000.0000 | 1000.0000 |  39.65 MB |

This is just simple string.Intern calls. It's not that much, but
it affects a startup time I care about.

The change could negatively affect the intern pool if random keys
are in the input.
@simonthum
Copy link
Author

simonthum commented May 17, 2025

I incorporated your comments.

But then I revisited my thinking about interning, performance and security. Real programs will have the keys they care about interned already, meaning we should prefer soft interning (IsInterned) to avoid loading the intern pool with crap. But this will be harder to benchmark.

I think it boils down to security. If you want YamlDotNet to be resilient to hostile input, I'm happy to switch to IsInterned.

@simonthum simonthum requested a review from EdwardCooke May 19, 2025 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants