-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
perf: replace json parser with orjson #71155
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -416,7 +416,7 @@ def _find_source_file_in_artifact_indexes(self): | |||
self._get_dist_matched_artifact_index_release_file() | |||
) | |||
if dist_matched_artifact_index_release_file is not None: | |||
raw_data = json.load(dist_matched_artifact_index_release_file.file.getfile()) | |||
raw_data = orjson.loads(dist_matched_artifact_index_release_file.file.getfile().read()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh interesting, this might be the leaked socket I've been looking for
each call to getfile
should be wrapped in context manager otherwise it leaks a file descriptor
no need to fix here though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #71155 +/- ##
==========================================
- Coverage 77.86% 77.74% -0.13%
==========================================
Files 6528 6528
Lines 290845 290849 +4
Branches 50338 50338
==========================================
- Hits 226467 226108 -359
- Misses 58128 58523 +395
+ Partials 6250 6218 -32
|
42515a0
to
eb7177e
Compare
@asottile-sentry Can you re-review? I had to make some changes due to failing tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eb7177e
to
b6e4e87
Compare
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
It seems we have a missing test coverage for this. Ref #71155 (comment) Fixes https://sentry-st.sentry.io/issues/5380029738/?project=1513938
PR reverted: c7ebc22 |
This reverts commit 63cb2b2. Co-authored-by: wedamija <6288560+wedamija@users.noreply.github.com>
Part of `orjson` move to get rid of `sentry.utils.json`
It seems we have a missing test coverage for this. Ref #71155 (comment) Fixes https://sentry-st.sentry.io/issues/5380029738/?project=1513938
This reverts commit 63cb2b2. Co-authored-by: wedamija <6288560+wedamija@users.noreply.github.com>
Part of
orjson
move to get rid ofsentry.utils.json