Extensive use of `Default` adds roughly 60ms of extra latency

[filipproch]

TLDR: Several request builders internally use ..Default::default() in their new() constructor to fill None values. The problem is that this actually constructs not only the None values, but also Client::default() (due to how ..Default::default() works).

Client::default() takes quite a long time (roughly 60ms on my system) and also I am quite sure its not intended to be constructed every time.

Reproduction / examples

It is the most pronounced on get_presigned_object_url, because that should be a very cheap call. On the current master branch, the following code:

let start = std::time::Instant::now();
let req = client.get_presigned_object_url("foo", "bar", Method.GET);
println("took {:?}", start.elapsed());

takes at least 60ms on my system.

Changing the implementation of GetPresignedObjectUrl::new like this: (replaces ..Default::default() with explicit assignments of None)

      Self {
          client,
          bucket,
          object,
          method,
          expiry_seconds: Some(DEFAULT_EXPIRY_SECONDS),
          extra_query_params: None,
          request_time: None,
          version_id: None,
          region: None,
      }

changes the time it takes to 2.28µs

Fix

I am happy to submit a patch/PR for fix, but I would like to first discuss the approach.

In my opinion, the best course of action would be to eliminate the default() on Client, because I think its too easy to then use it somewhere by mistake and cause built-in latency on key logic.

That said, given that would be API breaking change, at minimum we need to eliminate Default derive on structs like GetPresignedObjectUrl, and make sure its not added again in the future. And replace ..Default::default() with explicit None assignments.

Finally I suggest we add tests, at least for the get_presigned_object_url case, because that one is the most affected. In our case where we sign 50 URLs, the difference in how long this takes is from 4s to 4ms.

[HJLebbink]

Thanks for taking the time to write this up. We are also not too happy with how the defaults are handled. We have got a PR in the works (#184) that may addresses some of these issues. Could you check whether it partly resolves yours? With this Rust SDK we want to provide an efficient and robust API interface to some of MinIO’s functionality, and clearly, if something takes 4s instead of 4ms, we need to sort that out.

[filipproch]

Thanks for taking the time to write this up. We are also not too happy with how the defaults are handled. We have got a PR in the works (#184) that may addresses some of these issues. Could you check whether it partly resolves yours? With this Rust SDK we want to provide an efficient and robust API interface to some of MinIO’s functionality, and clearly, if something takes 4s instead of 4ms, we need to sort that out.

Thanks for the quick reply, I tried your PR (and checked the code) and I can confirm it doesn't have this issue.

What do you think would be the best way to prevent regression? For our codebase we added test that checks how long presign takes (as that should be very cheap and fast), but that doesn't cover other functions that used the same pattern, which as a result of the fix, got significantly faster as well. (eg DeleteObject).

And thank you for the effort you are putting into this SDK, a clean sheet SDK like this is very welcome compared to the one provided by AWS.

[HJLebbink]

@filipproch Could you put together a PR for the regression test you made? I might be able to use it for other calls too, to help prevent regressions.