server icon indicating copy to clipboard operation
server copied to clipboard

[PS-1471] Create Allocation Free `EncryptedStringAttribute` validation

Open justindbaur opened this issue 3 years ago • 0 comments

Type of change

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

Objective

We validate the shape of incoming encrypted strings but we don't actually do anything with the data so every bit of data that is allocated for that adds to the heap that then has to be deallocated later.

Method Branch EncryptedString Mean Error StdDev Gen0 Gen1 Allocated
IsValid encrypted-string-perf 2.Kl(...)uXg= [5616] 375.8 ns 4.47 ns 3.49 ns - - -
IsValid encrypted-string-perf 2.QmF(...)ydA== [52] 137.8 ns 2.79 ns 2.87 ns - - -
IsValid master 2.Kl(...)uXg= [5616] 15,731.6 ns 304.21 ns 362.14 ns 4.3640 0.2441 27425 B
IsValid master 2.QmF(...)ydA== [52] 314.5 ns 6.24 ns 6.93 ns 0.0839 - 528 B

NOTE: It's worth stating that a this attribute is called a few times in each request that uses them. For login it's used probably 4 times (Name, Username, Password, Uri) so the savings would be 4x at least what is shown in low [52] character input.

Code changes

  • bitwarden-server.sln: Added perf project so we can have a single place to add benchmarks.
  • perf/MicroBenchmarks/Core/EncryptedStringAttributeTests.cs: A benchmark that tests the current implementation of IsValid with two parameters, one that is a general "password" and one that is 5 paragraphs of lorem ipsum.
  • src/Core/Utilities/EncryptedStringAttribute.cs: Refactor of IsValid to be quicker and to have less allocations.
  • src/Core/Utilities/SpanExtensions.cs: Add Span extension for splitting a ReadOnlySpan by a certain character and returning whether it was successful and using out parameters to return both a span before and after the split character.
  • test/Core.Test/Utilities/EncryptedStringAttributeTests.cs: Added tests that more thourougly test the possible inputs to the validation and it's expected input
    • These were added before any changes to the underlying logic was made and all were successful against that with no uncovered branches (except for one unreachable branch).

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

justindbaur avatar Sep 09 '22 18:09 justindbaur