GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-103092: Isolate `_decimal` by CharlieZhao95 · Pull Request #103381 · python/cpython

Conversation

@CharlieZhao95

@CharlieZhao95 CharlieZhao95 commented Apr 8, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This PR is based on @erlend-aasland 's great work. Currently, most of the test cases can be passed, but still some work needs to be done. I make this PR as a draft so anyone can remind me of missing work or improve this code.

TODO

  • Test references leak and performance
  • Solve compilation warnings
  • Convert cond_map to heap type (not recorded in global-to-fix, but it seems like we should handle it)
  • Adapt multi-phase initialization
  • Remove redundant code and improve some dirty code
  • Issue: Isolate Stdlib Extension Modules #103092

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
erlend-aasland and others added 22 commits March 12, 2023 17:53
@erlend-aasland
Establish global state
60902dd
@erlend-aasland
Adapt PyDecContextManager_Type to heap type and add it to state
fa06750
@erlend-aasland
fixup! Adapt PyDecContextManager_Type to heap type and add it to state
f19c14c
@erlend-aasland
Adapt PyDecContext_Type to heap type and add it to state
cde5da9
@erlend-aasland
Adapt PyDecSignalDictMixin_Type to heap type and add it to state
90955ab
@erlend-aasland
fixup! Adapt PyDecSignalDictMixin_Type to heap type and add it to state
ae2ac48
@erlend-aasland
Adapt and add PyDec_Type & PyDecSignalDict_Type
eee3ef7
@erlend-aasland
Add DecimalTuple to state
f9c3573
@erlend-aasland
fixup! Add DecimalTuple to state
57316db
@erlend-aasland
Add DecimalException to state
30e4128
@erlend-aasland
fixup! Add DecimalException to state
f92e385
@CharlieZhao95
Add basic_context_template to state
8b6c0c7
@CharlieZhao95
Add extended_context_template to state
2ea6d98
@CharlieZhao95
Add default_context_template to state
7bf48be
@CharlieZhao95
Add context var to state
cd41d2c
@CharlieZhao95
fixup! Add default_context_template to state
44b0843
@CharlieZhao95
Add round_map to state
db7b988
@CharlieZhao95
Add Rational and SignalTuple to state
86170a2
@CharlieZhao95
Add external C-API functions to state
bc8df8f
@CharlieZhao95
port _decimal module to multi-initialization
4ff1074
@CharlieZhao95
Move global state to module state
8fba9e8
@CharlieZhao95
Add signal_map to state to fix error in decimal.Context.
be7fa2a
@bedevere-bot bedevere-bot mentioned this pull request Apr 8, 2023
Isolate Stdlib Extension Modules #103092
Closed
24 of 25 tasks
@bedevere-bot bedevere-bot added the awaiting review label Apr 8, 2023
@erlend-aasland

erlend-aasland commented Apr 8, 2023

Copy link
Copy Markdown
Contributor
  • Convert cond_map to heap type (not recorded in global-to-fix, but it seems like we should handle it)

I'm not sure I follow; cond_map is not a type.

Remove redundant code and improve some dirty code

Can you please spell this out? We cannot guess what you mean by this :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CharlieZhao95

CharlieZhao95 commented Apr 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author
  • Convert cond_map to heap type (not recorded in global-to-fix, but it seems like we should handle it)

I'm not sure I follow; cond_map is not a type.

Background

When I executed test_decimal.py after moving all variables in global-to-fix.csv into decimal_state, it raised an error:

Traceback (most recent call last):
  File "F:\CPython\github\cpython\Lib\test\test_decimal.py", line 5900, in <module>
    test_main(arith=True, verbose=True)
  File "F:\CPython\github\cpython\Lib\test\test_decimal.py", line 5840, in test_main
    init(C)
  File "F:\CPython\github\cpython\Lib\test\test_decimal.py", line 110, in init
    DefaultTestContext = m.Context(
                         ^^^^^^^^^^
KeyError: 'invalid signal dict'

I found that the place where the error occurs is PyDict_GetItemWithError(val, cm->ex) in dict_as_flags. This seems to be caused by the static signal_map whose members are assigned to types in module state.

So I guess signal_map should be moved to decimal_state (you can view my latest commit, maybe it's not the best solution). Likewise, cond_map[i].ex will be assigned to signal_map[i].ex, so it probably needs to be moved as well.

I'm not sure if these works are necessary, please correct me if I'm wrong :)

Remove redundant code and improve some dirty code

Can you please spell this out? We cannot guess what you mean by this :)

Of course, there is currently some unclean code. For example, signal_map_init or some code with FIXME comments, they work but are ugly. I will list the code that need to be modified after I have finished thinking. :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Apr 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Remove redundant code and improve some dirty code

Can you please spell this out? We cannot guess what you mean by this :)

Of course, there is currently some unclean code. For example, signal_map_init or some code with FIXME comments, they work but are ugly. I will list the code that need to be modified after I have finished thinking. :)

Is this code added by this PR, or code that is already there? For the latter, please do not include such changes in this PR. Instead, create an issue (unless there already is one), explain what you intend to do, and create a separate PR. This PR should focus on isolating _decimal, and nothing else.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CharlieZhao95

CharlieZhao95 commented Apr 10, 2023

Copy link
Copy Markdown
Contributor Author

If this code added by this PR, or code that is already there?

Sorry for not being clear. I mean the code added by this PR. I like to leave "FIXME" in my code to remind myself of potential work :)

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CharlieZhao95
solve warnings and remove redundant code
b130f56
@CharlieZhao95

CharlieZhao95 commented Apr 12, 2023

Copy link
Copy Markdown
Contributor Author

I tested the reference counts using erlend's script and they look well.

$./python measure.py
before=90491, after=93999
before=94000, after=94000
before=94000, after=94000
before=94000, after=94000
before=94000, after=94000

Also, I compared the execution time of the test_decimal between the current branch and python3.12, and there is no significant change in them. More detailed benchmark results can be viewed in Python Speed ​​Center after the merge.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CharlieZhao95

CharlieZhao95 commented Apr 12, 2023

Copy link
Copy Markdown
Contributor Author

Of course, there is currently some unclean code. For example, signal_map_init or some code with FIXME comments, they work but are ugly. I will list the code that need to be modified after I have finished thinking. :)

The code that I can think of that may need to be improved is as follows:

  1. If we call the mpd_setminalloc function multiple times, this function is called in decimal_exec, it will raise a warning. I temporarily use a static variable to ensure that it is not called multiple times. This doesn't seem pretty, any suggestions for improvement?
  2. Should we move signal_map into decimal_state(to fix the failing test cases, please see comment), is there a better solution?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland and others added 5 commits April 14, 2023 21:39
@erlend-aasland
Merge branch 'main' into isolate-decimal
39fe618
@CharlieZhao95
remove NDEBUG and static-local variables
a81da37
@CharlieZhao95
Add a NEWS entry.
6638ca7
@CharlieZhao95
Merge remote-tracking branch 'upstream/main' into isolate-decimal
7b05a94
@CharlieZhao95
Add Py_MOD_PER_INTERPRETER_GIL_SUPPORTED
68473a0
@CharlieZhao95
CharlieZhao95 marked this pull request as ready for review May 26, 2023 08:54
@CharlieZhao95
CharlieZhao95 requested a review from rhettinger as a code owner May 26, 2023 08:54
@rhettinger
rhettinger removed their request for review May 26, 2023 16:16
@kumaraditya303
Merge branch 'main' into isolate-decimal
e00dc55
kumaraditya303
kumaraditya303 reviewed May 28, 2023
View reviewed changes
Comment thread Modules/_decimal/_decimal.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@CharlieZhao95 @kumaraditya303
Update Modules/_decimal/_decimal.c
58f0049
Co-authored-by: Kumar Aditya <[email protected]>
@kumaraditya303
kumaraditya303 requested a review from erlend-aasland May 30, 2023 12:35
@kumaraditya303 kumaraditya303 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 1, 2023
@bedevere-bot

bedevere-bot commented Jun 1, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 58f0049 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 1, 2023
@erlend-aasland

erlend-aasland commented Jun 2, 2023

Copy link
Copy Markdown
Contributor

I'm sorry, but I don't have the bandwidth to review this in the near future.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kumaraditya303
kumaraditya303 reviewed Jun 16, 2023
View reviewed changes
Comment thread Modules/_decimal/_decimal.c
int n, mem;

assert(PyDecContext_Check(self));
decimal_state *state = get_module_state_by_def(Py_TYPE(self));

@kumaraditya303 kumaraditya303 Jun 16, 2023

Copy link
Copy Markdown
Contributor

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.

guard with Py_DEBUG

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

CharlieZhao95 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@erlend-aasland

erlend-aasland commented Jun 16, 2023

Copy link
Copy Markdown
Contributor

IMO, we should break this up into multiple PRs, like we did with _io; it will be easier to bisect if (when) bugs appear. See also PEP-687 for suggestions on how to break this up.

CharlieZhao95 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CharlieZhao95

CharlieZhao95 commented Jun 20, 2023

Copy link
Copy Markdown
Contributor Author

IMO, we should break this up into multiple PRs, like we did with _io; it will be easier to bisect if (when) bugs appear. See also PEP-687 for suggestions on how to break this up.

Thanks. This PR made a lot of changes at the same time, which might make it difficult to review. But the _deciaml module is simpler than the _io module, and I think 4~5 PRs are enough.

I plan to break the PR into the following parts:

  1. Establish a global module state and port some simple static types
  2. Move other global static variables to the global module state
  3. Convert the global module state to true module state

Potential work: Add Argument Clinic (It might be worth opening a new issue)

And I noticed that @kumaraditya303 has already done some review work, what do you think?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Jun 21, 2023

Copy link
Copy Markdown
Contributor

SGTM, ping me when you are done,

erlend-aasland and CharlieZhao95 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jun 25, 2023
Isolate the _decimal extension module #106078
Closed
gh-106078: Convert _decimal types to heap types #106079
Merged
@erlend-aasland
erlend-aasland marked this pull request as draft June 29, 2023 11:23
@bedevere-bot bedevere-bot removed the awaiting review label Jun 29, 2023
@kumaraditya303 kumaraditya303 closed this Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently will be requested when the pull request is marked ready for review ericsnowcurrently is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@CharlieZhao95 @erlend-aasland @bedevere-bot @kumaraditya303
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.