Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch for duplicate functions #233

Merged
merged 7 commits into from
Aug 17, 2023
Merged

Patch for duplicate functions #233

merged 7 commits into from
Aug 17, 2023

Conversation

Erotemic
Copy link
Member

Attempts to fix: #232

Adds in tests and also contains other code cleanup.

@Erotemic
Copy link
Member Author

@Theelx @notEvil One of the new tests related to duplicate functions is not passing on OSX, and I'm not sure why. It works on locally for me on Linux.

The invocation for the test in question is:

pytest -sv tests/test_explicit_profile.py -k test_explicit_profile_with_duplicate_functions

If either of you has an OSX device to test on, or can offer insights that would be helpful.

Could it be the case that the patch should only be applied to 3.11? (The 3.6 test is failining here and I'm not sure if other versions would pass).

@notEvil
Copy link

notEvil commented Aug 16, 2023

I don't, sry. As for the fix, maybe use .to_bytes(2, ... instead of * 2 to be safe.

@Theelx
Copy link
Collaborator

Theelx commented Aug 16, 2023

@Theelx @notEvil One of the new tests related to duplicate functions is not passing on OSX, and I'm not sure why. It works on locally for me on Linux.

The invocation for the test in question is:

pytest -sv tests/test_explicit_profile.py -k test_explicit_profile_with_duplicate_functions

If either of you has an OSX device to test on, or can offer insights that would be helpful.

Could it be the case that the patch should only be applied to 3.11? (The 3.6 test is failining here and I'm not sure if other versions would pass).

I do not have an OSX device, sorry. I think it is reasonable to only apply the patch to 3.11.

@Theelx
Copy link
Collaborator

Theelx commented Aug 16, 2023

I don't, sry. As for the fix, maybe use .to_bytes(2, ... instead of * 2 to be safe.

I don't think that's a good idea, as it'll just apply padding bytes with a value of 0 to the front of the bytecode, rather than padding with values of 9. See the docs for the length option here.

@notEvil
Copy link

notEvil commented Aug 16, 2023

Its not just padding, struct.unpack('H', (9).to_bytes(1, byteorder=sys.byteorder) * 2) != struct.unpack('H', (9).to_bytes(2, byteorder=sys.byteorder)). So if someone might validate the byte codes in the future, it won't be 9 but 2313

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #233 (b63ab15) into main (1b7c619) will increase coverage by 1.59%.
The diff coverage is 16.66%.

❗ Current head b63ab15 differs from pull request most recent head ebb1110. Consider uploading reports for the commit ebb1110 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
+ Coverage   51.77%   53.36%   +1.59%     
==========================================
  Files          11       11              
  Lines         817      832      +15     
  Branches      164      168       +4     
==========================================
+ Hits          423      444      +21     
+ Misses        335      329       -6     
  Partials       59       59              
Files Changed Coverage Δ
line_profiler/explicit_profiler.py 51.21% <11.11%> (-4.19%) ⬇️
...ine_profiler/autoprofile/ast_profle_transformer.py 75.00% <14.28%> (-10.30%) ⬇️
line_profiler/line_profiler.py 50.00% <50.00%> (+1.40%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b7c619...ebb1110. Read the comment docs.

@Erotemic
Copy link
Member Author

Erotemic commented Aug 16, 2023

The version condition didn't work.

Disabling OSX shows that everything else is passing. Going to check if it OSX with other versions also fails. EDIT: it does. It seems like it's just an OSX issue. No clue what it is.

@Erotemic
Copy link
Member Author

Erotemic commented Aug 17, 2023

The failing test in question is running the following code:

from line_profiler import profile

@profile
def func1(a):
    return a + 1

@profile
def func2(a):
    return a + 1

@profile
def func3(a):
    return a + 1

@profile
def func4(a):
    return a + 1

print(func1.__code__.co_code)
print(func2.__code__.co_code)
print(func3.__code__.co_code)
print(func4.__code__.co_code)

print(func1.__wrapped__.__code__.co_code)
print(func2.__wrapped__.__code__.co_code)
print(func3.__wrapped__.__code__.co_code)
print(func4.__wrapped__.__code__.co_code)

import ubelt as ub
print('profile._profile.code_map = {}'.format(ub.urepr(profile._profile.code_map, nl=1)))
for k, v in profile._profile.code_map.items():
    print(k.co_code)

func1(1)
func2(1)
func3(1)
func4(1)

The check is just that each of the 4 functions appear in the profile output:

assert 'Function: func1' in raw_output
assert 'Function: func2' in raw_output
assert 'Function: func3' in raw_output
assert 'Function: func4' in raw_output

The issue is on OSX func3 doesn't show up (although func1, func2, and func4 do).

The test includes debuggine lines to show the modified code binary. On OSX the dashboards show:

b'|\x00d\x01\x17\x00S\x00'
b'|\x00d\x01\x17\x00S\x00\t\x00\t\x00'
b'|\x00d\x01\x17\x00S\x00\t\x00\t\x00\t\x00'
b'|\x00d\x01\x17\x00S\x00\t\x00\t\x00\t\x00\t\x00'

But when I run on my linux machine the binary I get for the 4 functions is:

b'\x97\x00|\x00d\x01z\x00\x00\x00S\x00'
b'\x97\x00|\x00d\x01z\x00\x00\x00S\x00\t\t\t\t'
b'\x97\x00|\x00d\x01z\x00\x00\x00S\x00\t\t\t\t\t\t'
b'\x97\x00|\x00d\x01z\x00\x00\x00S\x00\t\t\t\t\t\t\t\t'

Does anything about these outputs give any hints?

@Erotemic
Copy link
Member Author

I think I figured it out. It was an unrelated issue. For some reason on OSX the measured time was zero, which meant it was filtered out by the stripzero condition. I change stripzero to look for zero hits instead of zero time, which is probably the correct behavior anyway. That seems to resolve the issue.

@Erotemic Erotemic merged commit ecf9c5b into main Aug 17, 2023
67 of 68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

co_code is malformed
3 participants