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

Improve (and test) memoryview reference counting #5510

Merged
merged 5 commits into from Jul 4, 2023

Conversation

da-woods
Copy link
Contributor

@da-woods da-woods commented Jul 4, 2023

  1. Avoid needing an indirection to align the atomic types. They should be aligned anyway, and this causes a notable slow-down.
  2. Don't use "volatile" typedef - volatile doesn't guarantee anything about atomicity and isn't needed here.
  3. Use explicit memory ordering. Won't make much difference on x86/x64 but may be faster on other architectures.
  4. Add a stress-test to try to make sure we haven't messed up.

Closes #5509

1. Avoid needing an indirection to align the atomic types. They
   should be aligned anyway, and this causes a notable slow-down.
2. Don't use "volatile" typedef - volatile doesn't guarantee anything
   about atomicity and isn't needed here.
3. Use explicit memory ordering. Won't make much difference on x86/x64
   but may be faster on other architectures.
4. Add a stress-test to try to make sure we haven't messed up.
@scoder
Copy link
Contributor

scoder commented Jul 4, 2023

Some internal warnings regarding utility code were moved:

[2] compiling (c/cy2) pure_warnings ... 
=== Expected: ===
12:10: Unknown type declaration 'Bar' in annotation, ignoring
15:16: Unknown type declaration 'stdint.bar' in annotation, ignoring
18:17: Unknown type declaration in annotation, ignoring
19:15: Unknown type declaration in annotation, ignoring
20:17: Unknown type declaration in annotation, ignoring
25:10: 'cpdef_method' redeclared
33:14: Unknown type declaration 'Bar' in annotation, ignoring
36:10: 'cpdef_cname_method' redeclared
37:20: Unknown type declaration 'stdint.bar' in annotation, ignoring
980:29: Ambiguous exception value, same as default return value: 0
1021:46: Ambiguous exception value, same as default return value: 0
1111:29: Ambiguous exception value, same as default return value: 0
=== Got: ===
12:10: Unknown type declaration 'Bar' in annotation, ignoring
15:16: Unknown type declaration 'stdint.bar' in annotation, ignoring
18:17: Unknown type declaration in annotation, ignoring
19:15: Unknown type declaration in annotation, ignoring
20:17: Unknown type declaration in annotation, ignoring
25:10: 'cpdef_method' redeclared
33:14: Unknown type declaration 'Bar' in annotation, ignoring
36:10: 'cpdef_cname_method' redeclared
37:20: Unknown type declaration 'stdint.bar' in annotation, ignoring
963:29: Ambiguous exception value, same as default return value: 0
1004:46: Ambiguous exception value, same as default return value: 0
1094:29: Ambiguous exception value, same as default return value: 0
FAIL

[4] compiling (c/cy2) fused_redeclare_T3111 ... ok
=== Expected: ===
25:10: 'cpdef_method' redeclared
36:10: 'cpdef_cname_method' redeclared
980:29: Ambiguous exception value, same as default return value: 0
980:29: Ambiguous exception value, same as default return value: 0
1021:46: Ambiguous exception value, same as default return value: 0
1021:46: Ambiguous exception value, same as default return value: 0
1111:29: Ambiguous exception value, same as default return value: 0
1111:29: Ambiguous exception value, same as default return value: 0
=== Got: ===
25:10: 'cpdef_method' redeclared
36:10: 'cpdef_cname_method' redeclared
963:29: Ambiguous exception value, same as default return value: 0
963:29: Ambiguous exception value, same as default return value: 0
1004:46: Ambiguous exception value, same as default return value: 0
1004:46: Ambiguous exception value, same as default return value: 0
1094:29: Ambiguous exception value, same as default return value: 0
1094:29: Ambiguous exception value, same as default return value: 0
FAIL

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Given the speed improvement that you report, I consider this a candidate for 3.0rc1. What do you think?

tests/memoryview/parallel_refcounting_stress_test.pyx Outdated Show resolved Hide resolved
@da-woods
Copy link
Contributor Author

da-woods commented Jul 4, 2023

Given the speed improvement that you report, I consider this a candidate for 3.0rc1. What do you think?

The speed improvement is ~20% on a very artificial test-case (I think it's the kind of code that people tend not to write in reality, because it's a bit slow). So it's decent, but won't really change anyone's calculations about what to write. #5508 will give a much more dramatic speed increase for the same case (and complements this rather than replaces it).

I think this PR is fairly low risk (to the extent that anyone really understands atomics), while the other PR needs a more careful look and a bit of polishing. So I'm personally inclined to merge this one.

If we do see issues with this, it'll be on non-x86 platforms, which I don't think we currently test.

@da-woods da-woods added this to the 3.0 milestone Jul 4, 2023
@da-woods da-woods merged commit 63cd89f into cython:master Jul 4, 2023
75 checks passed
@da-woods da-woods deleted the memoryview-ref-counting branch July 4, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Are we sure the memoryview atomics need aligning
2 participants