server icon indicating copy to clipboard operation
server copied to clipboard

[PS-1824] Minor performance improvements in ObfuscateEmail

Open stevenaw opened this issue 3 years ago • 3 comments

Type of change

- [ ] Bug fix
- [ ] New feature development
- [X] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

A few minor performance improvements in:

  • ApiHelpers.HandleAzureEvents()
  • CoreHelpers.ObfuscateEmail()

Code changes

  • ApiHelpers.HandleAzureEvents(): Removed a duplicate dictionary lookup by converting a ContainsKey() + indexer call to instead use TryGetValue()
  • CoreHelpers.ObfuscateEmail(): Convert the obfuscation work from using StringBuilder to be span-based and use string.Create()
Intel Core i7-8565U CPU 1.80GHz (Whiskey Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.100-rc.2.22477.23
  [Host]     : .NET 6.0.10 (6.0.1022.47605), X64 RyuJIT AVX2
  DefaultJob : .NET 6.0.10 (6.0.1022.47605), X64 RyuJIT AVX2


|             Method |                Email |      Mean |    Error |   StdDev |    Median |   Gen0 | Allocated |
|------------------- |--------------------- |----------:|---------:|---------:|----------:|-------:|----------:|
|      Obfuscate_New |       [email protected] |  82.22 ns | 1.721 ns | 4.768 ns |  83.34 ns | 0.0401 |     168 B |
| Obfuscate_Original |       [email protected] | 121.14 ns | 2.498 ns | 6.033 ns | 125.14 ns | 0.0725 |     304 B |
|      Obfuscate_New | reall(...)email [39] |  89.22 ns | 1.836 ns | 3.666 ns |  90.62 ns | 0.0631 |     264 B |
| Obfuscate_Original | reall(...)email [39] | 262.00 ns | 4.246 ns | 3.315 ns | 262.34 ns | 0.1392 |     584 B |

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

stevenaw avatar Nov 07 '22 03:11 stevenaw

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Nov 07 '22 03:11 CLAassistant

Thank you for your contribution! We've added this to our internal Community PR board for review. ID: PS-1824

bitwarden-bot avatar Nov 07 '22 03:11 bitwarden-bot

Ok, second attempt seems to have gone well. I was able to avoid the split altogether and simplify the code a bit by just writing the initial string into the new string's buffer and then overwriting the relevant part with a Fill() to make it effectively only two write operations.

|             Method |               Email |      Mean |     Error |    StdDev |    Median | Ratio | RatioSD |   Gen0 | Allocated | Alloc Ratio |
|------------------- |-------------------- |----------:|----------:|----------:|----------:|------:|--------:|-------:|----------:|------------:|
| Obfuscate_Original |      [email protected] |  98.75 ns |  2.043 ns |  4.080 ns |  99.51 ns |  1.00 |    0.00 | 0.0726 |     304 B |        1.00 |
|       Obfuscate_V1 |      [email protected] |  77.73 ns |  1.620 ns |  3.452 ns |  79.82 ns |  0.79 |    0.04 | 0.0401 |     168 B |        0.55 |
|       Obfuscate_V2 |      [email protected] |  20.36 ns |  0.473 ns |  0.977 ns |  20.90 ns |  0.21 |    0.01 | 0.0134 |      56 B |        0.18 |
|                    |                     |           |           |           |           |       |         |        |           |             |
| Obfuscate_Original | real(...)mail [105] | 425.51 ns | 24.272 ns | 70.804 ns | 437.07 ns |  1.00 |    0.00 | 0.2637 |    1104 B |        1.00 |
|       Obfuscate_V1 | real(...)mail [105] | 158.32 ns |  3.222 ns |  6.131 ns | 157.81 ns |  0.41 |    0.07 | 0.1261 |     528 B |        0.48 |
|       Obfuscate_V2 | real(...)mail [105] |  43.92 ns |  0.797 ns |  1.091 ns |  44.14 ns |  0.13 |    0.01 | 0.0554 |     232 B |        0.21 |

This should be ready for another look @justindbaur, thanks for your prompt review last time. Happy to have the chance to contribute.

EDIT: benchmarks updated to reflect latest commit.

stevenaw avatar Nov 14 '22 03:11 stevenaw

This seems to have stalled out. Anything I can do to help it along?

stevenaw avatar Jan 09 '23 00:01 stevenaw