Skip to content

380 - Compile list and dictionary parsing regexes #381

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.

Sign up for GitHub

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

Jump to bottom
Merged
da1910 merged 1 commit into from
Jul 12, 2023
Merged

380 - Compile list and dictionary parsing regexes #381

da1910 merged 1 commit into from
Jul 12, 2023

Conversation

@da1910
Copy link
Collaborator

@da1910 da1910 commented Jul 12, 2023 •
edited
Loading

Closes #380

This PR compiles the two regular expressions from ApiClient.__deserialize up front, this should improve performance a small but measureable amount.

@da1910 da1910 requested a review from Andy-Grigg July 12, 2023 13:20
@codecov
Copy link

codecov bot commented Jul 12, 2023 •
edited
Loading

Codecov Report

Merging #381 (3ed20d6) into main (70ec7c1) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #381      +/-   ##
==========================================
+ Coverage   94.28%   94.30%   +0.01%     
==========================================
  Files           7        7              
  Lines         770      772       +2     
==========================================
+ Hits          726      728       +2     
  Misses         44       44
Impacted Files Coverage Δ
src/ansys/openapi/common/_api_client.py 97.23% <100.00%> (+0.02%) ⬆️

Andy-Grigg
Andy-Grigg approved these changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@Andy-Grigg Andy-Grigg left a comment

Choose a reason for hiding this comment

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

Change seems fine, although there's some talk online that re.search is cached anyway. Are we sure this actually provides a significant speedup?

@da1910
Copy link
Collaborator Author

da1910 commented Jul 12, 2023

Change seems fine, although there's some talk online that re.search is cached anyway. Are we sure this actually provides a significant speedup?

Who knows, it's quite hard to tell. I do no longer see 300ms spent in re.compile, called 400000 times however!

@da1910 da1910 added this pull request to the merge queue Jul 12, 2023
@Andy-Grigg
Copy link
Contributor

Andy-Grigg commented Jul 12, 2023

Who knows, it's quite hard to tell. I do no longer see 300ms spent in re.compile, called 400000 times however!

Maybe the cache wasn't hitting for some reason. Something to keep an eye on though, I could potentially imagine an issue where even this improvement starts groaning and we might have to implement some non-regex solution instead. But as you say, ok at the moment.

Merged via the queue into main with commit f95d756 Jul 12, 2023
@da1910 da1910 deleted the fix/380_compile_regexes branch July 18, 2023 11:46
da1910 added a commit that referenced this pull request Jul 18, 2023
@da1910
Compile list and dictionary parsing regexes (#381)
98e1005
@da1910 da1910 mentioned this pull request Jul 18, 2023
Merged
da1910 added a commit that referenced this pull request Aug 13, 2023
@da1910
1.2 - Backport performance improvement to 1.2 (#385)
d5e2397 
* Compile list and dictionary parsing regexes (#381)

* Bump version

* Update ci_cd.yml

* Update conf.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Andy-Grigg Andy-Grigg Andy-Grigg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uncompiled regexes in ApiClient

3 participants

@da1910 @Andy-Grigg