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-103583: Isolate CJK codec modules by erlend-aasland · Pull Request #103869 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Apr 26, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: Isolate _multibytecodec #103583

Sorry, something went wrong.

Uh oh!

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

All reactions
@erlend-aasland
erlend-aasland requested a review from corona10 April 26, 2023 08:23
@bedevere-bot bedevere-bot mentioned this pull request Apr 26, 2023
Isolate _multibytecodec #103583
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Apr 26, 2023
@sobolevn sobolevn added the DO-NOT-MERGE label Apr 26, 2023
@erlend-aasland erlend-aasland removed the DO-NOT-MERGE label Apr 26, 2023
erlend-aasland added 5 commits April 26, 2023 14:22
@erlend-aasland
Pass codec iso. config to mbcodec API handlers
cb2c9e7
@erlend-aasland
Add cjk module state pointer to MultibyteCodec struct
409bf98
Lifetime should be guaranteed, since each MultibyteCodecObjects owns a
reference the cjk codec module that owns the memory of the
MultibyteCodec struct.
@erlend-aasland
Make it possible to specify cjk module specific state
fd6a2a0
@erlend-aasland
PoC: put big5 codec in cjk module state
eb44ae6
@erlend-aasland
PoC: iso2022 codecs
f6ed5df
@erlend-aasland
erlend-aasland force-pushed the isolate-cjk-alt2 branch from 664f961 to f6ed5df Compare April 26, 2023 13:13
@erlend-aasland
Add NEWS
ef983c4
@erlend-aasland
erlend-aasland marked this pull request as ready for review April 26, 2023 13:15
@erlend-aasland

erlend-aasland commented Apr 26, 2023

Copy link
Copy Markdown
Contributor Author

This approach exploits the dependency introduced in gh-103589; we know for sure that the _codec* extension modules will outlive _multibytecodec. This means we can store the cjk codec module state safely in the codec struct and modify the various codec handlers to accept the codec struct (iso. void *config). Then we add custom cjk module state where needed, and conveniently fetch it from the codec struct. (Quick and dirty explanation; see the code if what I wrote does not make sense 😎)

All reactions

Sorry, something went wrong.

Uh oh!

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

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

bedevere-bot commented Apr 26, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit ef983c4 🤖

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 Apr 26, 2023
@erlend-aasland

erlend-aasland commented Apr 26, 2023

Copy link
Copy Markdown
Contributor Author

BTW, I removed the initialized guards. I'm pretty sure we can do that, but I haven't verified it yet.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Apr 26, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Also, we can split out cb2c9e7 as a separate PR. That may be beneficial regarding to future bisects; it will also make this PR easier to review.

Similarly, f6ed5df can be split up to do the arg spec changes first, and then move stuff to module state afterwards.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Apr 26, 2023

Copy link
Copy Markdown
Contributor Author

@vstinner, you want to take a look? :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@corona10

corona10 commented Apr 26, 2023

Copy link
Copy Markdown
Member

@erlend-aasland I will take a look by tomorrow

erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
Merge branch 'main' into isolate-cjk-alt2
dec79a7
@erlend-aasland

erlend-aasland commented Apr 26, 2023

Copy link
Copy Markdown
Contributor Author

(Pulling in main to get the latest ref. leak fixes by Jelle)

All reactions

Sorry, something went wrong.

Uh oh!

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

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

bedevere-bot commented Apr 26, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit dec79a7 🤖

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 Apr 26, 2023
@erlend-aasland erlend-aasland changed the title gh-103583: Isolate CJK modules, alternative 2 gh-103583: Isolate CJK codec modules Apr 26, 2023
@erlend-aasland erlend-aasland linked an issue Apr 26, 2023 that may be closed by this pull request
Isolate _multibytecodec #103583
Closed
This was referenced Apr 26, 2023
gh-103583: Isolate CJK modules, alternative 1 #103868
Closed
gh-103583: Isolate CJK modules, alternative 3 #103870
Closed
corona10
corona10 reviewed Apr 27, 2023
View reviewed changes
Comment thread Modules/cjkcodecs/cjkcodecs.h Resolved

Uh oh!

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

corona10
corona10 approved these changes Apr 27, 2023
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

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.

LGTM, This is quite a complicated but nice approach. But nothing can be better than this.
Thanks for the hard work!

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Apr 27, 2023
@erlend-aasland

erlend-aasland commented Apr 27, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the review, Dong-hee! Yes, it is complicated, but as shown in my competing PRs, other approaches are even more complicated.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
erlend-aasland merged commit 8a0c7f1 into python:main Apr 27, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 27, 2023
@erlend-aasland
erlend-aasland deleted the isolate-cjk-alt2 branch April 27, 2023 13:02
@vstinner

vstinner commented May 4, 2023

Copy link
Copy Markdown
Member

Well done @erlend-aasland!

erlend-aasland reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented May 4, 2023

Copy link
Copy Markdown
Contributor Author

Well done @erlend-aasland!

Thanks, Victor :) This was a tricky one!

All reactions

Sorry, something went wrong.

Uh oh!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 approved these changes

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.

Isolate _multibytecodec

Uh oh!

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

5 participants

@erlend-aasland @bedevere-bot @corona10 @vstinner @sobolevn
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.