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-124855: Fix jit perf (and lejit) by Fidget-Spinner · Pull Request #126166 · python/cpython

Conversation

@Fidget-Spinner

@Fidget-Spinner Fidget-Spinner commented Oct 30, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

JIT builds are currently failing on main. This supersedes #126163. This wasn't caught because we don't run the JIT tests on pylifecycle.c

  • Issue: Don't allow the JIT and perf support to be active at the same time #124855

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Fidget-Spinner added 2 commits October 30, 2024 11:35
@Fidget-Spinner
Fix failing tier 2 interpreter tests on main
1f1b554
@Fidget-Spinner
Update jit.yml
636975d
@Fidget-Spinner
Fidget-Spinner requested review from brandtbucher and ericsnowcurrently as code owners October 30, 2024 03:37
@bedevere-app bedevere-app Bot added the awaiting core review label Oct 30, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Oct 30, 2024
Don't allow the JIT and perf support to be active at the same time #124855
Open
@Fidget-Spinner Fidget-Spinner mentioned this pull request Oct 30, 2024
gh-124855: Fix refleak in sys.activate_stack_trampoline #126163
Closed
@Fidget-Spinner
Fidget-Spinner requested a review from markshannon October 30, 2024 03:38
@Fidget-Spinner Fidget-Spinner added the skip news label Oct 30, 2024
@Fidget-Spinner
fix failing test
e1cb96f
@Fidget-Spinner
Fidget-Spinner requested a review from pablogsal October 30, 2024 08:16
@pablogsal

pablogsal commented Oct 30, 2024

Copy link
Copy Markdown
Member

I think we need to add skips to the tests in the Perf trampoline when the jit is activated
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal approved these changes Oct 30, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Oct 30, 2024
@pablogsal

pablogsal commented Oct 30, 2024

Copy link
Copy Markdown
Member

I have pushed a bunch of fixes to ensure we run the tests that check for anything perf without the JIT as we do in other places
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal reviewed Oct 30, 2024
View reviewed changes
Comment thread Lib/test/test_embed.py Outdated
self.check_all_configs("test_init_set_config", config,
api=API_ISOLATED)

@unittest.skipIf("-D_Py_JIT" in (sysconfig.get_config_var('PY_CORE_CFLAGS') or ''),

@pablogsal pablogsal Oct 30, 2024

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.

This fails on windows for some reason

Sorry, something went wrong.

Uh oh!

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

@pablogsal pablogsal Oct 30, 2024

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.

I think with my fix we can actually run the test so I am switching to that

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner Fidget-Spinner Oct 30, 2024

Copy link
Copy Markdown
Member Author

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.

Hmm I'm abit stumped. No clue why your fix isn't working :(.

Sorry, something went wrong.

Uh oh!

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

@pablogsal pablogsal Oct 30, 2024

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.

It's because the test is running on isolated mode and therefore it ignores the environment flags

Sorry, something went wrong.

Uh oh!

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

@pablogsal pablogsal Oct 30, 2024

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.

I pushed a fix that should do the trick

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pablogsal
Fix tests
8ed27a0 
Signed-off-by: Pablo Galindo <[email protected]>
@pablogsal
pablogsal force-pushed the fix-jit-perf branch from 37e9402 to 8ed27a0 Compare October 30, 2024 11:58
pablogsal and others added 3 commits October 30, 2024 13:54
@pablogsal
fix
f6e6745
@Fidget-Spinner
fix
c1a2ba4
@Fidget-Spinner
woops
491e161
@Fidget-Spinner

Fidget-Spinner commented Oct 30, 2024

Copy link
Copy Markdown
Member Author

Still failing on Windows... not sure why
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fidget-Spinner
try fix for Windows?
329ad20
@Eclips4

Eclips4 commented Oct 30, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Okay, I finally set up my Windows environment and it seems that there is no PY_CORE_CFLAGS. Perhaps we can't currently determine if JIT is enabled on non-configure builds. Maybe it's time to add a PY_JIT variable to pyconfig.h?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Eclips4 Eclips4 mentioned this pull request Oct 30, 2024
Add a Py_JIT_ENABLED variable to pyconfig.h #126204
Closed
@Fidget-Spinner Fidget-Spinner mentioned this pull request Oct 31, 2024
gh-126195: Use pthread_jit_write_protect_np on macOS #126196
Merged
@Eclips4 Eclips4 mentioned this pull request Oct 31, 2024
test_embed.InitConfigTests.test_initconfig_api fails when cpython is built with --enable-experimental-jit=interpreter #126255
Closed
@pablogsal

pablogsal commented Oct 31, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Maybe we can land a version of this that just ignores warning lines in stderr and then undo that hack when we have JIT detection on windows.

This way we can move on and avoid having to revert
Eclips4 and mpage 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.
@mpage

mpage commented Nov 1, 2024

Copy link
Copy Markdown
Contributor

Maybe we can land a version of this that just ignores warning lines in stderr and then undo that hack when we have JIT detection on windows.

This way we can move on and avoid having to revert

I put up #126302 as a possible work-around.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Eclips4 Eclips4 mentioned this pull request Nov 1, 2024
gh-126255: Ignore warning about JIT being deactivated when perf support is active in test_embed.InitConfigTests.test_initconfig_api #126302
Merged
@Eclips4

Eclips4 commented Nov 2, 2024

Copy link
Copy Markdown
Member

Closing it due to merge of #126302. Thank you Pablo and Ken.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Eclips4 Eclips4 closed this Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@brandtbucher brandtbucher Awaiting requested review from brandtbucher
@markshannon markshannon Awaiting requested review from markshannon

Uh oh!

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

Assignees

No one assigned

Labels

awaiting merge skip news

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

@Fidget-Spinner @pablogsal @Eclips4 @mpage
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.