Python to CLR string marshaling LRU cache. #538
Conversation
…oduces bugs when CPython freeing up enough objects.
|
@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.
|
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 to
a729ea4
Compare
a729ea4 to
355e30c
Compare
355e30c to
5136c85
Compare
|
@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. |
As requested here: #538 (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. |
| Marshal.FreeHGlobal(mem); | ||
| throw; | ||
| stringBytesCount = PyEncoding.GetByteCount(s); | ||
| mem = Marshal.AllocHGlobal(stringBytesCount + 4); |
There was a problem hiding this comment.
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.
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