Skip to content

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Nov 16, 2024

19% fewer traces created, no change in uops executed. No performance impact.

@brandtbucher brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-JIT labels Nov 16, 2024
@brandtbucher brandtbucher self-assigned this Nov 16, 2024
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

The warmup numbers are going to be wrong once #126816 is merged.

Do you want to get that merged first, then update this. Or the other way around?

@brandtbucher
Copy link
Member Author

I'll do that then this.

@brandtbucher
Copy link
Member Author

Fixed the tests (I also needed to change the logic a bit since the bytecode re-specializes before we JIT a new trace).

@brandtbucher brandtbucher merged commit 48c50ff into python:main Nov 20, 2024
59 checks passed
__run_using_command=[_strace_binary] + strace_flags)
__run_using_command=[_strace_binary] + strace_flags,
# Don't want to trace our JIT's own mmap and mprotect calls:
PYTHON_JIT="0",
Copy link
Contributor

@cmaloney cmaloney Nov 20, 2024

Choose a reason for hiding this comment

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

Guessing this showed up in the test_fileio test test_syscalls_read?

I have been keeping an eye on / working through a couple other issues where mmap(NULL) and mprotect show up, trying to figure out if just excluding those two from that test would be good/effective (memory allocation happening in reading files is fine / not what the test is focused on).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was wondering if this was susceptible to catching badly-timed arena allocations and such too. Might make sense to have an option to ignore the mmap/munmap/mprotect family of functions instead of this ad-hoc fix.

I'm just not familiar with how this is being used, and didn't want to change the meaning/behavior of the tests too dramatically.

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

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage topic-JIT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants