- Notifications
You must be signed in to change notification settings - Fork750
Python to CLR string marshaling LRU cache.#538
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…oduces bugs when CPython freeing up enough objects.
mention-bot commentedSep 6, 2017
@dmitriyse, thanks!@vmuriart,@tonyroberts,@tiran,@cgohlke and@hsoft, please review this. |
Codecov Report
@@ Coverage Diff @@## master #538 +/- ##=========================================- Coverage 77.12% 77.1% -0.02%========================================= Files 65 71 +6 Lines 5547 5857 +310 Branches 889 933 +44 =========================================+ Hits 4278 4516 +238- Misses 981 1033 +52- Partials 288 308 +20
Continue to review full report at Codecov.
|
codecovbot commentedSep 7, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #538 +/- ##==========================================- Coverage 76.9% 76.47% -0.44%========================================== Files 63 69 +6 Lines 5798 6015 +217 Branches 950 991 +41 ==========================================+ Hits 4459 4600 +141- Misses 1025 1081 +56- Partials 314 334 +20
Continue to review full report at Codecov.
|
Can you rebase this PR? looks like it was started out of a different PR that was already merged in. |
c02197d toa729ea4Comparea729ea4 to355e30cCompare355e30c to5136c85Compare@jakrivan can you add your comments as inline according to the code changes in this PR: |
@denfromufa - inline comment added:#625 However it's just one from many things that would need to be adjusted to support multi-domain usages. So feel free to reject - not to spoil the code with irrelevant comment. |
Note to self: This PR needs to be adjusted to the changes I made to PR#534, I renamed the |
| } | ||
| catch | ||
| { | ||
| // Do nothing with this. Very strange problem. |
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.
When does this happen?
| Marshal.FreeHGlobal(mem); | ||
| throw; | ||
| stringBytesCount = PyEncoding.GetByteCount(s); | ||
| mem = Marshal.AllocHGlobal(stringBytesCount + 4); |
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.
Why+4, wouldn't one additional byte be enough?
| return Runtime.IsPython3 | ||
| ? Instance.MarshalManagedToNative(s) | ||
| : Marshal.StringToHGlobalAnsi(s); | ||
| if (Runtime.IsPython3) |
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.
This means that your optimisation will only work at all for Python 3, is that necessary?
As a performance change this needs to be accompanied by a performance test, that sets up a baseline. E.g. first, a test must be merged, that sets the baseline performance goal without this change, then this PR should update the goal with the improved value. |
This is also probably unmergable in its current form, I have just kept it open for reference. |
What does this implement/fix? Explain your changes.
Adds string marshaling LRU cache (10k entries per marshaling type).
This greatly speedups mixed clr/py code. + Saves big amount of memory on large structures translation.
Does this close any currently open issues?
...
Any other comments?
This code is well tested in the high-load project. Memory usage was dramatically decreased after this improvement.
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG