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

Improve notification filter performance #15610

Merged
merged 5 commits into from
Mar 28, 2024
Merged

Improve notification filter performance #15610

merged 5 commits into from
Mar 28, 2024

Conversation

sebastienros
Copy link
Member

After the move to STJ some metrics went up. This fixed the lock contentions and large object heaps. Plus some other GC stats are also better. The creation of JsonSerializerOption uses a weak reference and locks internally so they shouldn't be created too often. The NotifyFilter was created then eagerly while not necessary. Now it's only on demand with some extra caching.

image

image

| application                             | main                      | perfnotify                |         |
| --------------------------------------- | ------------------------- | ------------------------- | ------- |
| Max CPU Usage (%)                       |                        99 |                        98 |  -1.01% |
| Max Cores usage (%)                     |                     2,760 |                     2,740 |  -0.72% |
| Max Working Set (MB)                    |                     1,543 |                     1,498 |  -2.92% |
| Max Private Memory (MB)                 |                     2,130 |                     1,810 | -15.02% |
| Build Time (ms)                         |                    31,814 |                    33,653 |  +5.78% |
| Start Time (ms)                         |                       433 |                       593 | +36.95% |
| Published Size (KB)                     |                   323,554 |                   323,551 |  -0.00% |
| Symbols Size (KB)                       |                     9,716 |                     9,715 |  -0.01% |
| .NET Core SDK Version                   | 9.0.100-preview.4.24177.8 | 9.0.100-preview.4.24177.8 |         |
| Max CPU Usage (%)                       |                        99 |                        98 |  -0.75% |
| Max Working Set (MB)                    |                     1,618 |                     1,620 |  +0.12% |
| Max GC Heap Size (MB)                   |                       904 |                       792 | -12.40% |
| Size of committed memory by the GC (MB) |                       772 |                       788 |  +2.11% |
| Max Number of Gen 0 GCs / sec           |                     13.00 |                     13.00 |   0.00% |
| Max Number of Gen 1 GCs / sec           |                      2.00 |                      1.00 | -50.00% |
| Max Number of Gen 2 GCs / sec           |                      1.00 |                      0.00 |         |
| Max Gen 0 GC Budget (MB)                |                     1,390 |                     1,396 |  +0.43% |
| Max Time in GC (%)                      |                     11.00 |                      2.00 | -81.82% |
| Max Gen 0 Size (B)                      |                10,430,512 |                10,945,440 |  +4.94% |
| Max Gen 1 Size (B)                      |                47,476,032 |                41,499,168 | -12.59% |
| Max Gen 2 Size (B)                      |                35,954,496 |                34,587,992 |  -3.80% |
| Max LOH Size (B)                        |                 9,173,976 |                   388,400 | -95.77% |
| Max POH Size (B)                        |                   694,136 |                   727,096 |  +4.75% |
| Max Allocation Rate (B/sec)             |             4,236,108,552 |             4,288,462,824 |  +1.24% |
| Max GC Heap Fragmentation (%)           |                    1,135% |                    1,713% | +50.93% |
| # of Assemblies Loaded                  |                       393 |                       393 |   0.00% |
| Max Exceptions (#/s)                    |                        13 |                        13 |   0.00% |
| Max Lock Contention (#/s)               |                       207 |                        45 | -78.26% |
| Max ThreadPool Threads Count            |                        41 |                        42 |  +2.44% |
| Max ThreadPool Queue Length             |                         6 |                         8 | +33.33% |
| Max ThreadPool Items (#/s)              |                    86,699 |                    87,439 |  +0.85% |
| Max Active Timers                       |                         8 |                         8 |   0.00% |
| IL Jitted (B)                           |                 2,717,327 |                 2,719,887 |  +0.09% |
| Methods Jitted                          |                    34,497 |                    34,531 |  +0.10% |

@sebastienros
Copy link
Member Author

@lahma I deleted ZStringWriter since it's in ZString library already, I assume at the time you added it (3 years ago) it was not public/available?

@lahma
Copy link
Contributor

lahma commented Mar 28, 2024

@lahma I deleted ZStringWriter since it's in ZString library already, I assume at the time you added it (3 years ago) it was not public/available?

Yes, it required someone to add it after being inspired by the need 😉

@MikeAlhayek MikeAlhayek merged commit ec3fa06 into main Mar 28, 2024
5 checks passed
@MikeAlhayek MikeAlhayek deleted the sebros/perfnotify branch March 28, 2024 20:19
Copy link
Contributor

@lahma lahma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's a closed PR, just adding couple thoughts...

// This is necessary as long as there will be string-based comparisons
// and the need of NotifyEntryComparer

var cache = _cache;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess nothing here prevents from Message changing and cache being invalid, private setter for properties (or being get-only) would make a different in API promises

@@ -175,7 +179,7 @@ private string SerializeNotifyEntry(NotifyEntry[] notifyEntries)
try
{
var protector = _dataProtectionProvider.CreateProtector(nameof(NotifyFilter));
var signed = protector.Protect(JConvert.SerializeObject(notifyEntries, _settings));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting here as I cannot comment on existing code...

  _existingEntries = messageEntries.Concat(_existingEntries).Distinct(new NotifyEntryComparer(_htmlEncoder)).ToArray();

Have I ever told that I don't like LINQ 😉 Could probably use HashSet directly and also ideally if possible, NotifyEntryComparer.Default (internal static readonly) if the serializer reference equals some known default, maybe internal NotifyEntryComparer.Create() could check such a thing and either create a new instance or use a default one

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.

3 participants