Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allocations overhead ? #843

Closed
mselee opened this issue Jul 30, 2023 · 4 comments · Fixed by #992
Closed

Allocations overhead ? #843

mselee opened this issue Jul 30, 2023 · 4 comments · Fixed by #992
Assignees

Comments

@mselee
Copy link

mselee commented Jul 30, 2023

First of all, thanks for the fantastic library.

While benchmarking pydantic v2 on some complex models with a lot of fields/nesting (unfortunately I can't share the models) I noticed abnormal performance due to allocations/deallocations:

flamegraph-before

@model_validator(mode='wrap') is used in these models but is dwarfed by the Clone and Drop calls. The (de-)allocations represent ~75% of the total run time.

I tried replacing the Box<CombinedValidator> with an Arc<RwLock<CombinedValidator>> for:

  • FunctionWrapValidator.validator
  • FunctionBeforeValidator.validator
  • FunctionAfterValidator.validator

After rebuilding with the patch, the (de-)allocations calls dropped:

flamegraph-after

Benchmark (1000 iterations)

Container Time Allocations Memory (Peak)
Box<CombinedValidator> ~13 sec 202000 66.4 KiB
Arc<RwLock<CombinedValidator>> ~3 sec 2000 1.6 KiB

But I'm not sure if this is actually sound (ref):

I would suggest using Arc where you are using Box, but unfortunately Validator::set_ref takes &mut self. This prevents calling it on Arc. While we could use Arc<RwLock> and acquire locks as needed, it would still likely be incorrect since currently all cloned validators are entirely independent, while with the Arc approach modifying the validator will also modifies everything which references it.

There's no set_ref in the code base anymore, but I guess the same concern is still valid for validator.complete() ?

Selected Assignee: @samuelcolvin

@samuelcolvin
Copy link
Member

Thanks so much for creating the issue, really interesting.

We were discussing Box just the other day and assuming it's overhead was tiny.

@davidhewitt what do you think?

@samuelcolvin
Copy link
Member

What was your benchmark of? Validation or validator construction?

@mselee
Copy link
Author

mselee commented Jul 30, 2023

It is something similar to this

class ComplexPydanticModel(BaseModel):
    # I also tried `defer_build=True` or removing some of `validate_xxx` but didn't make a noticeable difference
    model_config = ConfigDict(from_attributes=True, validate_default=True, validate_return=True)

    # fields/validators are omitted

class Response(BaseModel, Generic[T]):
    data: T


BenchmarkModel = Response[List[ComplexPydanticModel]]
data = {"data": data}

for _ in range(1000):
    BenchmarkModel.model_validate(data)

data is a list of complex ORM objects and from my testing the list size doesn't matter. What matters is the iterations in the loop i.e. for 100 iterations the runtime is barely affected, but it starts to be noticeable if you increase it:

Iterations Time
1000 ~13 sec
500 ~6 sec
100 ~3 sec

@davidhewitt
Copy link
Contributor

Thanks for the report and the proposed patch. Inspired by it I'm going a bit further and reducing away many cases where we clone validators (using probably just Rc<CombinedValidator> as we don't need the mutability any more).

I've got a patch which is almost there but I'm aiming to merge #867 first to make the patch simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants