Keats / jsonwebtoken

JWT lib in rust
MIT License
1.69k stars 271 forks source link

Improve deserialization performance around validation and its tests #202

Closed Ten0 closed 3 years ago

Ten0 commented 3 years ago

The claims validation was done via deserializing into a Map, which implies allocations/deallocations. This was done even if the map was not used afterwards.

This commit improves performance of the validation by never deserializing in a Map, and deserializing only when necessary, to a struct that typically only borrows from the original b64-decoded json string.

The validation function interface change required update to the tests, which are also made easier to read by using the serde_json::json! macro.

Keats commented 3 years ago

What does the change look like benchmark wise?

Ten0 commented 3 years ago

What does the change look like benchmark wise?

With the current benchmarks:

bench_decode            time:   [2.0427 us 2.0624 us 2.0839 us]
                        change: [-1.3756% -0.3096% +0.7734%] (p = 0.56 > 0.05)
                        No change in performance detected.

Benchmarking bench_dangerous_insecure_decode_with_validation: Collecting 100 samples                                                                                      bench_dangerous_insecure_decode_with_validation                        
                        time:   [1.0242 us 1.0286 us 1.0336 us]
                        change: [-4.2525% -2.5210% -1.0114%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  7 (7.00%) high mild
  3 (3.00%) high severe

However the more data there is in claims, the more significant this change becomes.

With the following benchmark for instance:

fn bench_dangerous_insecure_decode_with_validation(c: &mut Criterion) {
    #[derive(Deserialize)]
    struct Empty {}
    #[derive(Serialize)]
    struct Claims {
        exp: u64,
        nbf: u64,
        aud: &'static [&'static str],
        some_other_data: &'static str,
    }
    let token = encode(
        &Header::default(),
        &Claims { exp: 2324202280, nbf: 1472211880, aud: &["a", "b", "c", "d"], some_other_data: "2UszAuf5TNMbfn3LBpZqceVu2UszAuf5TNMbfn3LBpZqceVu2UszAuf5TNMbfn3LBpZqceVu2UszAuf5TNMbfn3LBpZqceVu2UszAuf5TNMbfn3LBpZqceVu" },
        &EncodingKey::from_secret("secret".as_ref()),
    )
    .unwrap();

    let validation = Validation {
        validate_exp: true,
        validate_nbf: true,
        aud: Some(["d", "e", "f", "g"].iter().map(|s| (*s).to_owned()).collect()),
        leeway: 1,
        ..Default::default()
    };
    c.bench_function("bench_dangerous_insecure_decode_with_validation", |b| {
        b.iter(|| {
            jsonwebtoken::dangerous_insecure_decode_with_validation::<Empty>(
                black_box(token.as_str()),
                black_box(&validation),
            )
            .unwrap()
        })
    });
}

The performance difference is the following:

Benchmarking bench_dangerous_insecure_decode_with_validation: Collecting 100 samples                                                                                      bench_dangerous_insecure_decode_with_validation                        
                        time:   [2.0310 us 2.0447 us 2.0582 us]
                        change: [-13.017% -12.267% -11.538%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  4 (4.00%) high mild
Keats commented 3 years ago

@Ten0 sorry for the delay, can you rebase?

Ten0 commented 3 years ago

@Ten0 sorry for the delay, can you rebase?

Done. Considering that you're squash-and-merging anyway I favored merging to keep the history in this PR.

Keats commented 3 years ago

Perfect, thanks!