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

Inspect tasks instead of assuming call signatures and return types #121

Merged
merged 2 commits into from
May 11, 2024

Conversation

griffinmilsap
Copy link
Collaborator

@griffinmilsap griffinmilsap commented May 10, 2024

Addresses #120.

Performance-wise there might be a barely significant speedup; possibly due to less hasattr checks and simpler branching/flow. All tests appear to pass.

Benchmark on my M1 Mac -- running test_perf.py -- many-dynamic-sizes:

Message size ASSUME (current impl.) INSPECT (new impl.)
32 b 14893 msgs/sec; 0.47 MB/s 15762 msgs/sec; 0.504 MB/s
512 b 14459 msgs/sec; 7.4 MB/s 16571 msgs/sec; 8.487 MB/s
8 kb 15572 msgs/sec; 127 MB/s 15951 msgs/sec; 130.67 MB/s
128 kb 10270 msgs/sec; 1346 MB/s 14399 msgs/sec; 1887 MB/s
2 Mb 4909 msgs/sec; 10296 MB/s 5463 msgs/sec; 11457 MB/s

Aside; I'm seeing some .. incomprehensible latency issues in the perf test. Something else to look into.

@cboulay
Copy link
Collaborator

cboulay commented May 10, 2024

Code looks good. I haven't tested it but I'm on the same arch as you so I don't think it'd help much ;)

@griffinmilsap
Copy link
Collaborator Author

@pperanich @cboulay I think this is good to go, I'm going to merge to dev; please do keep an ear to the ground if any of your systems break and give me a heads up before we push this to main

@griffinmilsap griffinmilsap merged commit 9996c7f into dev May 11, 2024
@cboulay cboulay deleted the 120_inspect_tasks branch June 9, 2024 17:58
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.

2 participants