| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Since all strings that will be rendered in the exception are actually ASCII messages (no localization), I'm considering switching from a PyDict storage to a _Py_hashtable_t structure but I don't know which one is the most efficient. With a _Py_hashtable_t structure, I can simply forget about Unicode PyObject * objects and deal with const char * objects only. I haven't checked more in details though since I already want to know whether the proposal performs well. |
Sorry, something went wrong.
|
_Py_hashtable_t will probably perform better, but it's not thread safe: is it possible for concurrent writes to be an issue here? |
Sorry, something went wrong.
|
No, we're only reading from the hash table. It is cached in the module's state and we only lookup strings by integer keys or pairs of integers. |
Sorry, something went wrong.
|
OpenSSL has fixed the issue upstream, but it looks like the release isn't due until April. |
Sorry, something went wrong.
|
Oups I misclicked on my mobile app. It's still a draft. |
Sorry, something went wrong.
|
Converted it back for you. IIRC the GitHub app doesn't let you mark something as a draft. |
Sorry, something went wrong.
|
Btw, I think we also have another issue with another code:
I didn't check whether this is still the case, but it's probably worth to check as well. |
Sorry, something went wrong.
|
Frustratingly, looks like OpenSSL again: https://github.com/openssl/openssl/blob/b049ce0e354011be075e620b9ba7cf4d7c8f9577/crypto/err/openssl.txt#L414 and https://github.com/openssl/openssl/blob/b049ce0e354011be075e620b9ba7cf4d7c8f9577/include/openssl/comperr.h#L28 I guess I'll go pester them again. Are there any other cases like this, or is this the last one? |
Sorry, something went wrong.
I think it's the last one. At least, I don't see more warnings in the CI. |
Sorry, something went wrong.
|
I found a few others while reporting the issue, FWIW. |
Sorry, something went wrong.
Thanks! I think we don't use those, hence I didn't have the warning (or maybe we're using it and something else is wrong in my warnings being emitted?) |
Sorry, something went wrong.
|
I'm going to abandon that specific PR. I may at some point refactor SSL but it's more pressing to fix other issues than this specific one (it didn't give much improvement and didn't really fix the issue users have). |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
I'd recommend also reviewing each commit separately due to the changes arising from logic extraction. I've also observed that we incorrectly use ERR_GET_REASON(p) on a custom error code. It's not really an issue because this is a no-op but I've added an assertion and removed the call for the semantics to match (we format the message using the error coming from ERR_peek_last_error() but the exception object is initialized with another error code that can also be obtained by SSL_get_error() or is a custom one).
I haven't benchmarked the branch, I just want to share a proposal. Note that refactoring the logic could help target the operations that take longer time.
cc @tarasko @kumaraditya303 @gpshead