Performance questions?

[samuelcolvin]

I'm keen to "run onto the spike" and find any big potential performance improvements in pydantic-core while the API can be changed easily.

I'd therefore love anyone with experience of rust and/or pyo3 to have a look through the code and see if I'm doing anything dumb.

Particular concerns:

• The "cast_as vs. extract" issues described in Add performance suggestions to docs PyO3/pyo3#2278 was a bit scary as I only found the solution by chance, are there any other similar issues with pyo3?
• generally copying and cloning values - am I copying stuff where I don't have to? In particular, is input or parts of input (in the case of a dict/list/tuple/set etc.) copied when it doesn't need to be?
• Similarly, could we use a PyObject instead of PyAny or visa-versa and improve performance?
• here and in a number of other implementations of ListInput and DictInput we do a totally unnecessary map, is this avoidable? Is this having a performance impact? Is there another way to give a general interface to the underlying datatypes that's more performance
• The code for generating models here seems to be pretty slow compared to other validators, can anything be done?
• Recursive models are slowing than I had expected, I thought it might be the use of RwLock that was causing the performance problems, but I managed to remove that (albeit in a slightly unsafe way) in simplifying recursive references #32 and it didn't make a difference. Is something else the problem? Could we remove Arc completely?
• lifetimes get pretty complicated, I haven't even checked if get a memory leak from running repeat validation, should we/can we change any lifetimes?

I'll add to this list if anything else comes to me.

More generally I wonder if there are performance improvements that I'm not even aware of? "What you don't know, you can't optimise"

@pauleveritt @robcxyz

[samuelcolvin]

I tried doing some performance profiling, see

https://github.com/samuelcolvin/pydantic-core/blob/7c9058e641549e816614a097148019c11feae95e/benchmarks/minimal.py#L3-L15

But I didn't find anything particularly interesting.

[Stranger6667]

Hi :) First of all, thank you for Pydantic!

Here are some ideas about allocations:

truncate - as it allocates in format!, but it could be avoided:

use std::fmt::Write;

macro_rules! truncate {
    ($out: expr, $value: expr) => {
        if $value.len() > 50 {
            write!($out, "{}...{}", &$value[0..25], &$value[$value.len() - 24..]);
        } else {
            $out.push_str(&$value);
        }
    };
}

Similarly, some push_str calls together with &format! could be replaced by write! (clippy should warn about it from Rust 1.61 btw):

output.push_str(&format!(", input_type={}", type_));

to

write!(output, ", input_type={}", type_);

There are a few more to_string calls that could be avoided:

let loc = self
    .location
    .iter()
    .map(|i| i.to_string())
    .collect::<Vec<String>>()
    .join(" -> ");

to:

let mut first = true;
for item in &self.location {
    if !first {
        output.push_str(" -> ");
        first = false
    }
    match item {
        LocItem::S(s) => output.push_str(&s),
        LocItem::I(i) => {
            output.push_str(itoa::Buffer::new().format(*i))
        },
    }
}

Required the itoa crate though - it would be helpful in a few more places (ryu would help with floats as well)

StrConstrainedValidator::_validation_logic if strip_whitespace and to_lower are true, then it will allocate twice, but only the latter actually requires allocating a new string.

Also, the signature implies an allocation, but it is not needed if e.g. min_length / max_length validation fails, so it might be better to use Cow instead (maybe also use Cow for Validator::get_name?)

py_error! uses format!, so I assume that calling to_string on its argument should not be necessary (like this one).

Are you ok with me submitting a PR for such things?

Static vs Dynamic dispatch. Is there any particular reason for using &'data dyn Input in the Validator trait? I'd assume it is possible to use &impl Input instead and avoid vtable indirection in cost of some compilation time.

The code for generating models here seems to be pretty slow compared to other validators, can anything be done?

inside with_prefix_location:

self.location = [location.clone(), self.location].concat();

One allocation could be avoided:

let mut new_location = location.clone();
new_location.extend_from_slice(&self.location);
self.location = new_location;

I'll take a look at other places during the weekend :)

[samuelcolvin]

Thanks so much, this is really interesting.

I didn't know allocation was particularly slow, I'll research it.

My initial guess is that most of these things will have a pretty small effect, e.g. so the truncate stuff is only called if you get an error and if print repr. I don't want to add more dependencies unless they allow a significant improvement.

The most significant thing is dyn Input Vs impl Input - if that makes a difference, it could make a big change.

PR very welcome.

[samuelcolvin]

Unfortunately using impl Input on Validators doesn't work because the trait needs to be object safe:

error[E0038]: the trait `Validator` cannot be made into an object
   --> src/validators/union.rs:12:22
    |
12  |     choices: Vec<Box<dyn Validator>>,
    |                      ^^^^^^^^^^^^^ `Validator` cannot be made into an object
    |
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
   --> src/validators/mod.rs:189:8
    |
181 | pub trait Validator: Send + Sync + fmt::Debug {
    |           --------- this trait cannot be made into an object...
...
189 |     fn validate<'s, 'data>(
    |        ^^^^^^^^ ...because method `validate` has generic type parameters
...
196 |     fn validate_strict<'s, 'data>(
    |        ^^^^^^^^^^^^^^^ ...because method `validate_strict` has generic type parameters
    = help: consider moving `validate` to another trait
    = help: consider moving `validate_strict` to another trait

[danielSanchezQ]

I would like to help too (been keen about helping this project for some time 😃 ). I would take a look and if I see some low hanging fruit will be posting (small) PRs. that is ok?
Are stylistic changes welcome too (thinks like iterators vs loops etc)?

[samuelcolvin]

Amazing yes please.

Are stylistic changes welcome too (thinks like iterators vs loops etc)?

yes, but my third priority (after safety and speed) is readability, particularly for python developers - e.g. me. So if changes make it technically more correct rust but make it harder to read for notices, and have no other impact, I might be inclined to refuse them.

Best to create some small PRs and I'll comment.

[mejrs]

Are stylistic changes welcome too (thinks like iterators vs loops etc)?

That's not just a stylistic choice - avoiding bound checks means iterators can often be faster than index based loops.

Similarly, could we use a PyObject instead of PyAny or visa-versa and improve performance?

PyObject is just the owned version of PyAny - use the former whenever you need to keep around references to Python objects.

generally copying and cloning values - am I copying stuff where I don't have to? In particular, is input or parts of input (in the case of a dict/list/tuple/set etc.) copied when it doesn't need to be?

Where is this?

here and in a number of other implementations of ListInput and DictInput we do a totally unnecessary map, is this avoidable? Is this having a performance impact? Is there another way to give a general interface to the underlying datatypes that's more performance

If you just want to provide an interface, consider using a visitor rather than returning an iterator. This also avoids the boxing, but it doesn't really matter for performance I think.

For example, you have:

impl<'data> ListInput<'data> for &'data PyList {
    fn input_iter(&self) -> Box<dyn Iterator<Item = &'data dyn Input> + 'data> {
        Box::new(self.iter().map(|item| item as &dyn Input))
    }
    // ...
}

But you could also accept a closure to run over each item:

impl<'data> ListInput<'data> for &'data PyList {
    fn visit(&self, visitor: impl FnMut(???))  {
        self.iter().for_each(visitor)
    }
}

Or if you prefer something more typed:

trait Validator{
    fn validate(???)
}

impl<'data> ListInput<'data> for &'data PyList {
    fn visit(&self, visitor: impl Validator)  {
        self.iter().for_each(|item| visitor.validate(item))
    }
}

More generally I wonder if there are performance improvements that I'm not even aware of? "What you don't know, you can't optimise"

I mainly see some papercuts, not any actual performance crimes.

For example, in apply_validator, you have:

let loc = if key_loc {
    vec![key.to_loc(), "[key]".to_loc()]
} else {
    vec![key.to_loc()]
};
for err in line_errors {
    errors.push(err.with_prefix_location(&loc));
}

But if you fix the with_prefix_location to take a reference to a slice, rather than a reference to a Vec, you can avoid creating the vecs altogether:

if key_loc {
    let locs = [key.to_loc(), "[key]".to_loc()];
    for err in line_errors {
        errors.push(err.with_prefix_location(&locs));
    }

} else {
     let loc = key.to_loc();
     for err in line_errors {
        errors.push(err.with_prefix_location(std::slice::from_ref(&loc)));
    }
};

Note that the above comment holds true in general: because taking &Vec has more indirection and requires the caller to provide a vec rather than any continuous sequence you don't want this

fn foo(blahs: &Vec<Blah>){}

but this:

fn foo(blahs: &[Blah]){}

Recursive models are slowing than I had expected, I thought it might be the use of RwLock that was causing the performance problems, but I managed to remove that (albeit in a slightly unsafe way) in #32 and it didn't make a difference. Is something else the problem?

    fn set_ref(&mut self, name: &str, validator_weak: ValidatorWeak) -> PyResult<()> {
        unsafe { Arc::get_mut_unchecked(&mut self.validator_arc).set_ref(name, validator_weak) }
    }

Functions like these are incredibly dangerous, they enable unsynchronized shared mutability. At least make set_ref unsafe. You could try using parking_lot's synchronization primitives - they are a bit faster.

Could we remove Arc completely?

Arc is just for sharing things. If you want to share things, you will need it or Rc.

lifetimes get pretty complicated, I haven't even checked if get a memory leak from running repeat validation, should we/can we change any lifetimes?

Do you mean Rust's lifetime annotations?

[mejrs]

Also, I recommend to do benchmarking as part of your CI workflows, so you can identify performance regressions (or improvements :) ) quickly.

[adriangb]

cc @woodruffw

[samuelcolvin]

I got some really helpful suggestions from @hoodmane, noting them here before i forgot:

• Use an enum for validators, then use enum_dispatch, instead of dyn Validator, same for InputList
• Use a slot table instead of weak references for recursive references
• use tp_new to create classes.

[woodruffw]

This is a small thing, but getattr with constant strings can be slightly optimized by using intern!: https://docs.rs/pyo3/latest/pyo3/prelude/struct.Py.html#method.getattr

For example, this:

https://github.com/samuelcolvin/pydantic-core/blob/7c9058e641549e816614a097148019c11feae95e/src/validators/model_class.rs#L29

could become:

let new_method = class.getattr(intern!("__new__"))?;

...to save an allocation. I can make a small PR for these, if you'd like.

Edit: Actually looks like one of these is already interned, so this would be a single line change 🙂

[Stranger6667]

I didn't know allocation was particularly slow, I'll research it.
My initial guess is that most of these things will have a pretty small effect, e.g. so the truncate stuff is only called if you get an error and if print repr. I don't want to add more dependencies unless they allow a significant improvement.

Those are indeed micro-optimizations for some specific scenarios, but sometimes they may yield some nice cumulative effect :) I.e. if there is an opportunity to avoid heap allocations I usually prefer to take it

Here is another small Idea that may help a bit in some scenarios (with some cost of slowing down the others) - using a bitvector (e.g. u64) instead of a set for storing what fields were hit inside ModelValidator::validate (fields are ordered there). It might work for validators with <65 fields (which I think is quite common), for others, it could fall back to a regular set. The check_extra branch will require an actual set as those field names are arbitrary. The final Ok branch will require creating a set. It might not improve the happy path case when there are no validation errors, but maybe it could be considered if those other scenarios would need some extra performance (bitwise operations should be orders of magnitude cheaper than set operations)

[samuelcolvin]

@woodruffw thanks so much for the input. I think on this occasion this is superseded by #43.

I'll look at all the other suggestions here when I have time.