FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Python to CLR string marshaling LRU cache. by dmitriyse · Pull Request #538 · pythonnet/pythonnet · GitHub

/ pythonnet Public

Python to CLR string marshaling LRU cache. - #538

Closed
dmitriyse wants to merge 19 commits into
pythonnet:masterfrom
dmitriyse:string-marshaling-cache
ConversationCommits (19)ChecksFiles changed
Closed

Python to CLR string marshaling LRU cache.#538
dmitriyse wants to merge 19 commits into
pythonnet:masterfrom
dmitriyse:string-marshaling-cache

Conversation

dmitriyse commented Sep 6, 2017

Copy link
Copy Markdown
Contributor

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.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

mention-bot commented Sep 6, 2017

Copy link
Copy Markdown

@dmitriyse, thanks! @vmuriart, @tonyroberts, @tiran, @cgohlke and @hsoft, please review this.

codecov Bot commented Sep 7, 2017

Copy link
Copy Markdown

Codecov Report

Merging #538 into master will decrease coverage by 0.01%.
The diff coverage is 79.33%.

@@            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
Flag Coverage Δ
#setup_linux 73.64% <73.8%> (-2.07%) ⬇️
#setup_windows 76.28% <75.48%> (-0.19%) ⬇️
Impacted Files Coverage Δ
src/runtime/nativecall.cs 100% <ø> (+2.22%) ⬆️
src/runtime/pythonengine.cs 80.35% <ø> (+1.66%) ⬆️
src/runtime/pyscope.cs 57.92% <100%> (-0.79%) ⬇️
src/runtime/assemblymanager.cs 89.83% <100%> (+0.05%) ⬆️
src/runtime/genericutil.cs 86.36% <100%> (+0.42%) ⬆️
src/runtime/metatype.cs 71.55% <100%> (ø) ⬆️
src/runtime/moduleobject.cs 85% <100%> (-0.06%) ⬇️
src/runtime/delegatemanager.cs 88.18% <100%> (-0.42%) ⬇️
src/runtime/typemanager.cs 84.16% <100%> (+0.13%) ⬆️
src/runtime/classderived.cs 88.41% <100%> (+0.07%) ⬆️
... and 25 more

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 af62a1a...f7bf1dd. Read the comment docs.

codecov Bot commented Sep 7, 2017 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #538 into master will decrease coverage by 0.43%.
The diff coverage is 64.77%.

@@            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
Flag Coverage Δ
#setup_linux 68.55% <ø> (ø) ⬆️
#setup_windows 75.71% <64.77%> (-0.41%) ⬇️
Impacted Files Coverage Δ
src/runtime/assemblymanager.cs 87.03% <ø> (ø) ⬆️
src/runtime/pyobject.cs 39.01% <0%> (-1.33%) ⬇️
src/runtime/runtime.cs 86.95% <100%> (+0.23%) ⬆️
src/runtime/perf_utils/RawImmutableMemBlock.cs 48.14% <48.14%> (ø)
src/runtime/CustomMarshaler.cs 58.09% <48.93%> (+0.56%) ⬆️
src/runtime/perf_utils/RawMemoryFifoDictionary.cs 65.21% <65.21%> (ø)
...runtime/perf_utils/EncodedStringsFifoDictionary.cs 70.37% <70.37%> (ø)
src/runtime/perf_utils/FifoDictionary.cs 70.83% <70.83%> (ø)
...rc/runtime/perf_utils/EncodingGetStringPolyfill.cs 75% <75%> (ø)
src/runtime/perf_utils/RawMemUtils.cs 84.48% <84.48%> (ø)
... and 7 more

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 5ee234a...49d0d89. Read the comment docs.

vmuriart commented Sep 22, 2017

Copy link
Copy Markdown
Contributor

Can you rebase this PR? looks like it was started out of a different PR that was already merged in.

dmitriyse force-pushed the string-marshaling-cache branch from c02197d to a729ea4 Compare September 22, 2017 01:19

dmitriyse commented Sep 22, 2017

Copy link
Copy Markdown
Contributor Author

This PR depends on one non merged PR #534

Also I rebased all PR that depends on merged PRs #518 and #519

dmitriyse force-pushed the string-marshaling-cache branch from a729ea4 to 355e30c Compare November 16, 2017 22:17
dmitriyse force-pushed the string-marshaling-cache branch from 355e30c to 5136c85 Compare January 12, 2018 18:31

den-run-ai commented Feb 10, 2018

Copy link
Copy Markdown
Contributor

@jakrivan can you add your comments as inline according to the code changes in this PR:

https://github.com/pythonnet/pythonnet/pull/538/files

JanKrivanek added a commit to JanKrivanek/pythonnet that referenced this pull request Feb 12, 2018
Added inline comment about dangers of multidomain usage
46a943f 
As requested here: pythonnet#538 (comment)
JanKrivanek mentioned this pull request Feb 12, 2018
Merged
4 tasks

JanKrivanek commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

@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.

filmor pushed a commit that referenced this pull request Oct 16, 2018
Added inline comment about dangers of multidomain usage
9ba5be5 
As requested here: #538 (comment)
filmor pushed a commit to filmor/pythonnet that referenced this pull request Oct 16, 2018
Added inline comment about dangers of multidomain usage
677281e 
As requested here: pythonnet#538 (comment)

filmor commented Oct 17, 2018

Copy link
Copy Markdown
Member

Note to self: This PR needs to be adjusted to the changes I made to PR #534, I renamed the TestsSuite class.

filmor reviewed Oct 18, 2018
View reviewed changes
Comment thread src/runtime/CustomMarshaler.cs
}
catch
{
// Do nothing with this. Very strange problem.

filmor Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

When does this happen?

Comment thread src/runtime/CustomMarshaler.cs
Marshal.FreeHGlobal(mem);
throw;
stringBytesCount = PyEncoding.GetByteCount(s);
mem = Marshal.AllocHGlobal(stringBytesCount + 4);

filmor Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why +4, wouldn't one additional byte be enough?

Comment thread src/runtime/CustomMarshaler.cs
return Runtime.IsPython3
? Instance.MarshalManagedToNative(s)
: Marshal.StringToHGlobalAnsi(s);
if (Runtime.IsPython3)

filmor Oct 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This means that your optimisation will only work at all for Python 3, is that necessary?

filmor added the next label Nov 15, 2018
filmor added this to the 2.4.0 milestone Nov 15, 2018
filmor mentioned this pull request Jan 18, 2019
Closed
filmor modified the milestones: 2.4.0, 2.4.1 Apr 12, 2019

lostmsu commented Mar 3, 2020

Copy link
Copy Markdown
Member

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.

filmor commented Mar 4, 2020

Copy link
Copy Markdown
Member

This is also probably unmergable in its current form, I have just kept it open for reference.

lostmsu removed this from the 2.4.1 milestone Mar 4, 2020
filmor removed the next label Jun 9, 2020
filmor closed this Aug 9, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

filmor filmor left review comments
tonyroberts Awaiting requested review from tonyroberts
den-run-ai Awaiting requested review from den-run-ai
vmuriart Awaiting requested review from vmuriart

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL