ActiveLogin.Authentication icon indicating copy to clipboard operation
ActiveLogin.Authentication copied to clipboard

Add benchmarks

Open PeterOrneholm opened this issue 5 years ago • 2 comments

Is your feature request related to a problem? Please describe. We have plans on doing performance improvements, but to ensure we focus on the right thing, we should add a couple of benchmarks.

What area is it related to We could of course add benchmarks to all of these packages, but I would say this prio order would make most sense due to their usage/impact:

  1. ActiveLogin.Authentication.BankId.Api
  2. ActiveLogin.Authentication.BankId.AspNetCore
  3. ActiveLogin.Authentication.GrandId.Api
  4. ActiveLogin.Authentication.GrandId.AspNetCore
  5. ActiveLogin.Authentication.BankId.AspNetCore.Azure
  6. ActiveLogin.Authentication.BankId.AspNetCore.QRCoder

Describe the solution you'd like Add benchmarks using BenchmarkDotNet for the most used/critical paths.

My idea is that we start with focus on the BankID API wrapper and one or two API endpoints in the BankID.AspNetCore package.

ActiveLogin.Authentication.BankId.Api Write at least one benchmark for each method in the BankIdApiClient.cs class. Fake the response using a custom message handler to ensure we don't stress test the BankID API. Ideas on how to do this can be found here. (But make sure to use SocketsHttpHandler instead.)

ActiveLogin.Authentication.BankId.AspNetCore Write at least one test for the Status and Initialize action in the BankIdApiController.cs class. They are used the most. Make sure to use a fake IBankIdApi implementation to remove that out of the equation.

Additional context Having these in place we can start looking at potential performance improvements. I think that we should add benchmarks over time, but an initial PR would focus on the things described above (ActiveLogin.Authentication.BankId.Api and parts of ActiveLogin.Authentication.BankId.AspNetCore).

PeterOrneholm avatar Dec 13 '19 09:12 PeterOrneholm

I implemented the first draft in #278 which covers all public methods in BankIdApiClient.cs.

The results on my machine:

BenchmarkDotNet=v0.12.1, OS=macOS Catalina 10.15.6 (19G73) [Darwin 19.6.0] Intel Core i7-7920HQ CPU 3.10GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores .NET Core SDK=3.1.100 [Host] : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT DefaultJob : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT

Method Mean Error StdDev
AuthAsync 2.533 ms 0.3460 ms 1.020 ms
SignAsync 2.776 ms 0.3599 ms 1.061 ms
CollectAsync 2.584 ms 0.3701 ms 1.091 ms
CancelAsync 2.295 ms 0.3787 ms 1.116 ms

This is a really good idea to start with benchmarks @PeterOrneholm but I think just adding them to the project won't benefit us without proper integration to CI and analysing them.

I suggest that we add on more CI pipeline that runs benchmarks (since it can take really long time) which can be triggered when there is a merge to master branch. We can allocate all the reports and analyse them in Azure DevOps dashboard. What do you think @PeterOrneholm ?

nikolaykrondev avatar Aug 07 '20 11:08 nikolaykrondev