-
Notifications
You must be signed in to change notification settings - Fork 2
Fixed Mojo + benchmark script #1
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
base: main
Are you sure you want to change the base?
Conversation
b8491b9
to
fef4d22
Compare
Hey @uberkael , |
Hello @uberkael, Thanks for the review! |
Yes, |
while i < len(s): | ||
|
||
if i < len(s) - 1 and py.abs(ord(s[i]) - ord(s[i + 1])) == 32: |
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 could see why importing from python was not necessary in my other codes but here this should be working just fine, no?
@@ -1,22 +1,14 @@ | |||
from python import Python | |||
|
|||
def lengthOfLastWord(enterword: String): |
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'm trying to understand why you changed the defining keyword here from "def" to "fn". I mean it worked fine with "def", any reason for that?
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.
fn in mojo allow to use types, which enable the compiler to optimize more effectively.
Thanks for the PR. I found it strange that Mojo is ~ x20 slower. I was just googling benchmarks and stumbled upon this. I've never used Mojo and these results almost swayed me from doing so. These examples are definitely misleading with the compilation overhead and lack of optimizations. |
I updated the code and removed all print() statements. The results:
|
|
|
||
|
||
fn main(): | ||
for _ in range(100000): | ||
_ = lengthOfLastWord("Hello World") |
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.
You will need to write keep(lengthOfLastWord("Hello World")
, otherwise lenghOfLastWord
gets DCE'd.
from benchmark import keep
...
This is incorrect. The benchmarks only measure the Python startup overhead and the Mojo compilation overhead; the test programs are small compared to those times.
I'm not an expert, but out of curiosity, I did update the Mojo code and compiled it before running the hyperfine call with a shell script
bench.sh
.As you can see all the times are mostly the same, meaning all the Leetcode examples are too small for a benchmark, even if mojo compiled is 9x faster in this case.