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

Automatization for fetching inputs, use nightly, logic simplifications #1

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

msaelices
Copy link

@msaelices msaelices commented Oct 12, 2024

  • Make it work with Mojo-nightly
  • Fix run.sh script
  • Typo fixed in the README.md
  • Add headers to the benchmarks for readability
  • Simplify day01 logic loops by using Mojo stdlib iterators
  • Script that download all the inputs from adventofcode
  • No need for variadic list, which is mostly used for packing arguments
  • Simplify, the day01.mojo looping by using Span. This needs for this PR to be merged into the nightly release, as the negative steps are not working in Mojo: [stdlib] Span slicing with negative step modularml/mojo#3650
  • More pythonic Mojo code in day02.mojo. It's slower but more readable. It will be faster overtime as Mojo dicts and strings become fasters.
  • Get rid of wildcard imports
  • Fix edge cases in day03.py
  • Fix the calls to store() in the array.mojo file The unsafe pointers have relaxed signatures in the .store() method now

Signed-off-by: Manuel Saelices <[email protected]>
This needs for this PR to be merged into the nightly release, as the negative steps are not working in Mojo: modularml/mojo#3650

Signed-off-by: Manuel Saelices <[email protected]>
It's slower but more readable. It will be faster overtime as Mojo dicts
and strings become fasters. E.g. see
modularml/mojo#3436
modularml/mojo#3528
modularml/mojo#3615

Signed-off-by: Manuel Saelices <[email protected]>
Signed-off-by: Manuel Saelices <[email protected]>
Signed-off-by: Manuel Saelices <[email protected]>
Signed-off-by: Manuel Saelices <[email protected]>
The unsafe pointers have relaxed signatures in the .store() method now
Copy link
Owner

@p88h p88h left a comment

Choose a reason for hiding this comment

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

First of all, thanks for helping with cleaning some of this stuff !
Not sure how you got here but I appreciate it.

Most of this change is very simple - the only part I have some reservations about is the rewrite of day2 to utilize builtin parsing functions, which are still really slow, PTAL at the comment below.

Other than that, should need just some minor fixes.

for i in range(len(words)):
self.add(words[i])
for w in words:
self.add(w[])
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks - I fixed the language problems when upgrading to 24.5 but never got around to removing these relics.

@@ -89,11 +84,11 @@ fn main() raises:
a1 += lsum

# Construct matchers for all words. When looking backwards, the words have to be reversed.
# Fun fact - VariadicList apparently can hold literals, but cannot hold Strings.
# Fun fact - List apparently can hold literals, but cannot hold Strings.
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is no longer true (used to be way back when) -> please remove comment if migrating to List<> works now

cnt = int(toks[i * 2])
col = toks[i * 2 + 1]
counter[col] = max(counter[col], cnt)
return counter
Copy link
Owner

Choose a reason for hiding this comment

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

While I appreciate the direction, this repo's point is to build fast solutions. While it's nice that Mojo now has some collection builtins, split() was there before and I'm not using it since it's abysmally slow.

This task is a perfect example of that problem - with this approach it's about 8 times slower than the 'hacky but fast' solution was (which was on par with PyPy), and it's now ~2.5 times slower than Python.

The hand-crafted parsers that work over raw memory are not really easy (or efficient) in Python, so this is still some way in which I was able to squeeze some performance of Mojo. OTOH sure, this approach shows performance gaps in the std library. Perhaps add this code as a _slow variant of this solution to highlight this, but keep this version as-is (or modified to adjust for some QoL improvements in recent Mojo as in other bits.

try:
c = ord(lines[y][x])
except IndexError:
c = 0
Copy link
Owner

Choose a reason for hiding this comment

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

So this is probably more of an effect of assumptions on what is contained in lines and how they are loaded - the code in Python is rather simple and does not do the trailing newline check. Rather than a try/catch here you could fix this by removing the last element of lines[] after it's loaded, if it's empty.

@@ -1,6 +1,6 @@
[project]
authors = ["Staszek Pasko <[email protected]>"]
channels = ["conda-forge", "https://conda.modular.com/max"]
channels = ["conda-forge", "https://conda.modular.com/max-nightly"]
Copy link
Owner

Choose a reason for hiding this comment

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

My experiences with nightly have been mixed recently and would prefer this repo to work under the stable API... that said 24.5 has not been very stable as well....

Are there any benefits to nightly I don't know about ?

return Span[UInt8, __lifetime_of(self)](
unsafe_ptr=self.ptr,
len=self.size,
)
Copy link
Owner

Choose a reason for hiding this comment

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

An unrelated question but this code suffered a bit from lifetime-related issues... are these lifetime parametrizations useful enough now to tell Mojo that eg. keep stuff around in memory more consistently ?

I understand here it's just needed because Spans work like that, but I never figured out how to make the StringSlices here behave in such a way to make sure their lifetime prolonged the lifetime of the parser they originated from (since they depend on that memory)

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