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

extract vs cast_as #2

Closed
wants to merge 2 commits into from
Closed

extract vs cast_as #2

wants to merge 2 commits into from

Conversation

samuelcolvin
Copy link
Member

Confusingly this PR is going from fast to slow.

Performance (from running python benchmarks/run.py):

With cast_as:

plain:
    validate_str_recursive: 2292.42µs

pydantic_core._pydantic_core:
    validate_str_recursive: 729.15µs

With extract:

plain:
    validate_str_recursive: 2328.11µs

pydantic_core._pydantic_core:
    validate_str_recursive: 3782.37µs

@adamreichold in case this helps.

@@ -101,9 +101,9 @@ pub fn validate_str_recursive<'py>(
to_lower: bool,
to_upper: bool,
) -> PyResult<&'py PyAny> {
if let Ok(list) = value.cast_as::<PyList>() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can reproduce this locally and it turns out that exactly these two calls to extract make the difference. (The other ones above do not.)

(It would be nice to loose the second here to keep this PR focused on the extract-vs-cast-as issue.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks so much for looking into it.

Is there anything I can do to help here?

On balance, to avoid profiling every change, I guess I'd prefer to use cast_as where possible - I feels more semantically correct and AFAIK it's never slower (???). So I'll just close this PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reproduced the problem in an isolated PyO3 benchmark now. So I think you can close this and also revoke my access to the repository. Everything else will need to be handled in the PyO3 repository.

@samuelcolvin
Copy link
Member Author

Closing this as there's more detail on PyO3/pyo3#2278.

Thanks for your help.

@samuelcolvin samuelcolvin deleted the slow branch April 20, 2022 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants