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

Slightly reorganize initialization code #438

Merged
merged 5 commits into from
Dec 19, 2019
Merged

Slightly reorganize initialization code #438

merged 5 commits into from
Dec 19, 2019

Conversation

asvetlov
Copy link
Member

@asvetlov asvetlov commented Dec 14, 2019

Curiously, #432 problem seems gone.
I don't understand clearly why but the code doesn't contain explicit tricks, it is clearly readable at least.

URL of RTD document: https://multidict.readthedocs.io/en/cleanup/ Documentation Status

@codecov
Copy link

codecov bot commented Dec 14, 2019

Codecov Report

Merging #438 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #438   +/-   ##
=======================================
  Coverage   75.97%   75.97%           
=======================================
  Files           5        5           
  Lines         487      487           
=======================================
  Hits          370      370           
  Misses        117      117

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c76da36...229b581. Read the comment docs.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Dec 14, 2019
@asvetlov
Copy link
Member Author

Hmm, actually it doesn't fix but masks a problem.
Import error is raised and the system switches to pure python under pyinstaller.
The difference is that import doesn't crash now.

Need to investigate what behavior was in previous multidict versions? Maybe pyinstaller was never good with the multidict?

@iemelyanov
Copy link
Contributor

iemelyanov commented Dec 14, 2019

Hmm, actually it doesn't fix but masks a problem.
Import error is raised and the system switches to pure python under pyinstaller.
The difference is that import doesn't crash now.

Need to investigate what behavior was in previous multidict versions? Maybe pyinstaller was never good with the multidict?

Previous versions v4.6.1 and lower works correct because pyinstaller not use C-extension.

Copy link
Contributor

@iemelyanov iemelyanov left a comment

Choose a reason for hiding this comment

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

LGTM

@asvetlov asvetlov changed the title Slightly reorganize initialization code a little Slightly reorganize initialization code Dec 16, 2019
@rtd-helper
Copy link

rtd-helper bot commented Dec 16, 2019

WebSocket is not open: readyState 3 (CLOSED)

@Varriount
Copy link

@asvetlov The fact that #432 is gone for no perceivable reason is worrying. To me, that indicates that there was some form of undefined behavior causing trouble. Do tests run with memory sanitizer/valgrind enabled?

@asvetlov
Copy link
Member Author

@Varriount as I see the reason for the error is: pyinstaller splits multidict in two locations: Python files are bundled with all other sources into executable image and multidict folder with compiled shared library.
When the shared library tries to import multidict._multidict_base python module the import fails because of the split. Seems like pyinstaller supports a bunch of hooks for popular libraries to avoid this problem but I didn't dig into the project too deeply.

The PR "fixes" the problem to correctly process the import failure and switching back to pure-python implementation (which is much slower but still works).

Also, #443 has fixed another crash unrelated to this issue.

1 similar comment
@asvetlov
Copy link
Member Author

@Varriount as I see the reason for the error is: pyinstaller splits multidict in two locations: Python files are bundled with all other sources into executable image and multidict folder with compiled shared library.
When the shared library tries to import multidict._multidict_base python module the import fails because of the split. Seems like pyinstaller supports a bunch of hooks for popular libraries to avoid this problem but I didn't dig into the project too deeply.

The PR "fixes" the problem to correctly process the import failure and switching back to pure-python implementation (which is much slower but still works).

Also, #443 has fixed another crash unrelated to this issue.

@asvetlov
Copy link
Member Author

asvetlov commented Dec 19, 2019

The Inlining of pair_list adds 15-25% boost.
Other inlines seem don't add anything but I'll keep it for the sake of consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants