[stdlib] Faster repr() logic for String, StringRef, UInt, DType and others #3736

msaelices · 2024-11-03T23:56:35Z

Implement StringRef.__repr__()
Faster path.stat() implementation using Writer
Faster UInt.__repr__()
Faster Error.__repr__()
Faster DType.__repr__()

Signed-off-by: Manuel Saelices <[email protected]>

This improves the representation to be like String.__repr__() Signed-off-by: Manuel Saelices <[email protected]>

Signed-off-by: Manuel Saelices <[email protected]>

JoeLoser

Do you mind looking into the pathlib test failure as a result of this change? See this CI run for example.

stdlib/src/os/fstat.mojo

Signed-off-by: Manuel Saelices <[email protected]>

…e OS Signed-off-by: Manuel Saelices <[email protected]>

msaelices · 2024-11-07T22:20:27Z

Do you mind looking into the pathlib test failure as a result of this change? See this CI run for example.

I think the check is flaky as it depends on the OS, and I don't have the MacOS to test it. I've just removed off-topic checks

msaelices · 2024-11-07T22:21:34Z

@JoeLoser could you please TAL?

JoeLoser · 2024-11-09T19:20:22Z

@JoeLoser could you please TAL?

Yep, CI is failing though - do you mind looking into that first?

JoeLoser · 2024-11-09T19:20:43Z

stdlib/src/collections/string.mojo

+            value: The string to represent.
+        """
+        self._value = StringSlice[ImmutableAnyOrigin](
+            unsafe_from_utf8_ptr=value.unsafe_ptr(),


Suggested change

unsafe_from_utf8_ptr=value.unsafe_ptr(),

ptr=value.unsafe_ptr(),

stdlib/src/collections/string.mojo

JoeLoser · 2024-11-10T04:03:50Z

stdlib/src/os/fstat.mojo

@@ -46,7 +46,7 @@ fn _constrain_unix():


 @value
-struct stat_result(Stringable):
+struct stat_result(Stringable, Writable):


Suggestion Split this change to stat_result to be Writable along with its tests to a separate PR, please.

The goal of this commit was to have a faster representation of path.stat() which uses stat_result underneath, which was already representable but slow, so to me was in the same scope of "Faster repr() logic" msaelices@950d36a

Do you still want me to split those changes into a new PR?

I understand the scope of the commit, but in the future, I'd recommend teasing apart the two ideas (applying Writable to a struct and making repr() faster for a few types). They're orthogonal things in my mind and could have been two separate PRs. It's fine for this one though, but something to consider in the future 😄

martinvuyk

Hi, I've been working on faster repr and ascii logic for about 3 weeks now, see PR #3686. This does not add much speed really.

I think you can still merge the other improvements you made on this PR. But see the _repr function that I've been working on, there are several other things that need to exist as helpers to make this much faster.

martinvuyk · 2024-11-10T13:47:17Z

stdlib/src/collections/string.mojo

+        var quote = "'"
+        if len(self._value) > 0 and self._value[0] == "'":
+            quote = '"'


this is not how python works

>>> print(repr("hi")) 'hi' >>> print(repr("'hi'")) "'hi'" >>> print(repr("a 'hi' a")) "a 'hi' a"

I cannot find the issue here. This is a side-by-side comparison:

The current nightly implementation does this:

var use_dquote = False for s in self: use_dquote = use_dquote or (s == "'")

which checks if any character in the string is ', and that makes the __repr__ change to using double quotes. The logic above only checks if the first character is ' and would fail in the 3rd example.

Not sure where the command line execution you're showing here comes from, but if it's from the results of this code then I must've misread the logic 😄

martinvuyk · 2024-11-10T13:48:03Z

stdlib/src/collections/string.mojo

+        writer.write(quote)
+        for s in self._value:
+            if s == "\\":
+                writer.write(r"\\")
+            elif s == "\t":
+                writer.write(r"\t")
+            elif s == "\n":
+                writer.write(r"\n")
+            elif s == "\r":
+                writer.write(r"\r")
+            else:
+                var codepoint = ord(s)
+                if isprintable(codepoint):
+                    writer.write(s)
+                elif codepoint < 0x10:
+                    writer.write(hex(codepoint, prefix=r"\x0"))
+                elif codepoint < 0x20 or codepoint == 0x7F:
+                    writer.write(hex(codepoint, prefix=r"\x"))
+                else:  # multi-byte character
+                    writer.write(s)
+        writer.write(quote)


This isn't really much faster. It still uses write hex() and doesn't alloc before doing everything

Yes, it was not a great optimization. I only mapped the top-level constructor to use .write() but did not spend the needed work on making it way faster.

Signed-off-by: Manuel Saelices <[email protected]>

Co-authored-by: Joe Loser <[email protected]> Signed-off-by: Manuel Saelices <[email protected]>

We will take advantage of a faster StringRef.__repr__() when modular#3686 was merged Signed-off-by: Manuel Saelices <[email protected]>

waiting for being merged here: modular#3686 Signed-off-by: Manuel Saelices <[email protected]>

msaelices · 2024-11-12T15:33:12Z

Hi, I've been working on faster repr and ascii logic for about 3 weeks now, see PR #3686. This does not add much speed really.

I think you can still merge the other improvements you made on this PR. But see the _repr function that I've been working on, there are several other things that need to exist as helpers to make this much faster.

Ohh! What an awesome optimization. I've reverted the changes here:

msaelices@cb66ec9
msaelices@958106b

msaelices · 2024-11-12T15:39:36Z

@martinvuyk, @JoeLoser could you please TAL?

martinvuyk

LGTM

martinvuyk · 2024-11-12T19:30:59Z

stdlib/src/os/fstat.mojo

+        writer.write("os.stat_result(")
+        writer.write("st_mode=", self.st_mode)
+        writer.write(", st_ino=", self.st_ino)
+        writer.write(", st_dev=", self.st_dev)
+        writer.write(", st_nlink=", self.st_nlink)
+        writer.write(", st_uid=", self.st_uid)
+        writer.write(", st_gid=", self.st_gid)
+        writer.write(", st_size=", self.st_size)
+        writer.write(", st_atime=", str(self.st_atimespec))
+        writer.write(", st_mtime=", str(self.st_mtimespec))
+        writer.write(", st_ctime=", str(self.st_ctimespec))
+        writer.write(", st_birthtime=", str(self.st_birthtimespec))
+        writer.write(", st_blocks=", self.st_blocks)
+        writer.write(", st_blksize=", self.st_blksize)
+        writer.write(", st_rdev=", self.st_rdev)
+        writer.write(", st_flags=", self.st_flags)
+        writer.write(")")


Suggestion:
The String.write_bytes() reserves the amount of bytes to join the next item every time, in the future we should be able to constrain traits and know if they implement for example as_bytes() and be able to reserve the total amount of bytes beforehand. So this suggestion doesn't make a difference currently but maybe in the future:

these can all go in the same function call separated by commas. They will be formatted similarly by the Mojo formatter so it won't make much difference in the aesthetics other than the name of the field won't be directly to the left, still it's just a matter of personal preference and not performance currently 🤷‍♂️ .

(This would all become redundant if somebody reserves enough capacity for the whole os.stat result before calling write_to())

JoeLoser · 2024-11-14T16:47:45Z

!sync

modularbot · 2024-11-14T17:21:30Z

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

modularbot · 2024-11-15T06:29:18Z

Landed in b27d751! Thank you for your contribution 🎉

[External] [stdlib] Make `StringRef` Representable - [X] Implement `StringRef.__repr__()` - [X] Implement `Writable` for `os.stat_result` Co-authored-by: Manuel Saelices <[email protected]> Closes #3736 MODULAR_ORIG_COMMIT_REV_ID: f8fe27aa7b0274fa27665b8cd0b19af22f3cac3b

[External] [stdlib] Make `StringRef` Representable - [X] Implement `StringRef.__repr__()` - [X] Implement `Writable` for `os.stat_result` Co-authored-by: Manuel Saelices <[email protected]> Closes modular#3736 MODULAR_ORIG_COMMIT_REV_ID: f8fe27aa7b0274fa27665b8cd0b19af22f3cac3b

[External] [stdlib] Make `StringRef` Representable - [X] Implement `StringRef.__repr__()` - [X] Implement `Writable` for `os.stat_result` Co-authored-by: Manuel Saelices <[email protected]> Closes modular#3736 MODULAR_ORIG_COMMIT_REV_ID: f8fe27aa7b0274fa27665b8cd0b19af22f3cac3b Signed-off-by: Manuel Saelices <[email protected]>

[External] [stdlib] Make `StringRef` Representable - [X] Implement `StringRef.__repr__()` - [X] Implement `Writable` for `os.stat_result` Co-authored-by: Manuel Saelices <[email protected]> Closes #3736 MODULAR_ORIG_COMMIT_REV_ID: f8fe27aa7b0274fa27665b8cd0b19af22f3cac3b

msaelices added 5 commits November 3, 2024 23:12

Implement StringRef.__repr__()

19b23a1

Signed-off-by: Manuel Saelices <[email protected]>

[stdlib] Faster path.stat() implementation using Writer

950d36a

Signed-off-by: Manuel Saelices <[email protected]>

Optimize String.__repr__() by using Writer

c6c9277

Signed-off-by: Manuel Saelices <[email protected]>

[stdlib] Use StringRepr for StringRef.__repr__()

f1472b9

This improves the representation to be like String.__repr__() Signed-off-by: Manuel Saelices <[email protected]>

Add changelog entry

3fbb842

Signed-off-by: Manuel Saelices <[email protected]>

msaelices requested a review from a team as a code owner November 3, 2024 23:56

msaelices changed the title ~~[stdlib] Better repr() logic for String and StringRef~~ [stdlib] Faster repr() logic for String and StringRef Nov 3, 2024

JoeLoser reviewed Nov 4, 2024

View reviewed changes

stdlib/src/os/fstat.mojo Outdated Show resolved Hide resolved

JoeLoser requested a review from jackos November 4, 2024 19:27

msaelices added 4 commits November 7, 2024 22:38

[stdlib] Faster UInt.__repr__()

3c2129f

Signed-off-by: Manuel Saelices <[email protected]>

[stdlib] Faster Error.__repr__()

91f0b80

Signed-off-by: Manuel Saelices <[email protected]>

[stdlib] Faster DType.__repr__()

db15abe

Signed-off-by: Manuel Saelices <[email protected]>

[stdlib] Simpler stat_result.__repr__() logic

348b434

Signed-off-by: Manuel Saelices <[email protected]>

msaelices force-pushed the better-representable branch from 8be18b2 to 348b434 Compare November 7, 2024 21:47

msaelices added 4 commits November 7, 2024 22:48

[stdlib] Hopefully fix the test_stat() test in MacOS

97a7a96

Signed-off-by: Manuel Saelices <[email protected]>

Merge branch 'nightly' into better-representable

4b67bfc

[stdlib] Simpler String.__repr__() logic

40e2787

Signed-off-by: Manuel Saelices <[email protected]>

[stdlib] Remove two failing checks in test_stat() which depends on th…

696339c

…e OS Signed-off-by: Manuel Saelices <[email protected]>

msaelices requested a review from JoeLoser November 7, 2024 22:21

msaelices changed the title ~~[stdlib] Faster repr() logic for String and StringRef~~ [stdlib] Faster repr() logic for String, StringRef, UInt, DType and others Nov 8, 2024

Merge branch 'nightly' into better-representable

fb93c6b

JoeLoser reviewed Nov 10, 2024

View reviewed changes

JoeLoser self-assigned this Nov 10, 2024

martinvuyk suggested changes Nov 10, 2024

View reviewed changes

msaelices added 3 commits November 12, 2024 14:17

Merge branch 'nightly' into better-representable

7e4da67

[stdlib] Adapt the code to s/unsafe_from_utf8_ptr/ptr refactoring

3ab5ef0

Signed-off-by: Manuel Saelices <[email protected]>

[stdlib] Replace StringRepr to _StringRepr

32a507d

Signed-off-by: Manuel Saelices <[email protected]>

msaelices and others added 2 commits November 12, 2024 14:50

Fix typo in _StringRepr docstring

1ee14b1

Co-authored-by: Joe Loser <[email protected]> Signed-off-by: Manuel Saelices <[email protected]>

[stdlib] Do not use _StringRepr for StringRef.__repr__()

cb66ec9

We will take advantage of a faster StringRef.__repr__() when modular#3686 was merged Signed-off-by: Manuel Saelices <[email protected]>

msaelices force-pushed the better-representable branch from c85a70a to cb66ec9 Compare November 12, 2024 15:30

[stdlib] Revert the StringRepr logic as there is a much better approach

958106b

waiting for being merged here: modular#3686 Signed-off-by: Manuel Saelices <[email protected]>

martinvuyk approved these changes Nov 12, 2024

View reviewed changes

JoeLoser approved these changes Nov 14, 2024

View reviewed changes

modularbot added the imported-internally Signals that a given pull request has been imported internally. label Nov 14, 2024

modularbot added the merged-internally Indicates that this pull request has been merged internally label Nov 14, 2024

modularbot added the merged-externally Merged externally in public mojo repo label Nov 15, 2024

modularbot closed this Nov 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stdlib] Faster repr() logic for String, StringRef, UInt, DType and others #3736

[stdlib] Faster repr() logic for String, StringRef, UInt, DType and others #3736

msaelices commented Nov 3, 2024 •

edited

Loading

JoeLoser left a comment

msaelices commented Nov 7, 2024

msaelices commented Nov 7, 2024

JoeLoser commented Nov 9, 2024

JoeLoser Nov 9, 2024

JoeLoser Nov 10, 2024

msaelices Nov 12, 2024

JoeLoser Nov 14, 2024

martinvuyk left a comment

martinvuyk Nov 10, 2024

msaelices Nov 12, 2024 •

edited

Loading

martinvuyk Nov 12, 2024

martinvuyk Nov 10, 2024

msaelices Nov 12, 2024

msaelices commented Nov 12, 2024

msaelices commented Nov 12, 2024

martinvuyk left a comment

martinvuyk Nov 12, 2024 •

edited

Loading

JoeLoser commented Nov 14, 2024

modularbot commented Nov 14, 2024

modularbot commented Nov 15, 2024

	unsafe_from_utf8_ptr=value.unsafe_ptr(),
	ptr=value.unsafe_ptr(),

[stdlib] Faster repr() logic for String, StringRef, UInt, DType and others #3736

[stdlib] Faster repr() logic for String, StringRef, UInt, DType and others #3736

Conversation

msaelices commented Nov 3, 2024 • edited Loading

JoeLoser left a comment

Choose a reason for hiding this comment

msaelices commented Nov 7, 2024

msaelices commented Nov 7, 2024

JoeLoser commented Nov 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvuyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msaelices Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msaelices commented Nov 12, 2024

msaelices commented Nov 12, 2024

martinvuyk left a comment

Choose a reason for hiding this comment

martinvuyk Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

JoeLoser commented Nov 14, 2024

modularbot commented Nov 14, 2024

modularbot commented Nov 15, 2024

msaelices commented Nov 3, 2024 •

edited

Loading

msaelices Nov 12, 2024 •

edited

Loading

martinvuyk Nov 12, 2024 •

edited

Loading