-
Notifications
You must be signed in to change notification settings - Fork 905
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
Optimize []byte
to string
conversions
#4289
Conversation
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.
It looks like this PR won't handle a case like this correctly:
b := []byte{...}
s := string(b) // convert to string (without alloc)
b[0] = 5 // this modifies the string
Basically it doesn't just need a check whether the byte slice escapes, but also that it is read-only after it is converted to a string.
2b3c622
to
0ce0e60
Compare
Rebased and pushed a new version, that also improves the read-only checks to catch more usages. I still need to fine-comb the table I added for this; however, local tests ( |
Signed-off-by: L. Pereira <l.pereira@fastly.com>
Turns out we were missing quite a few optimizations because some instructions that should be read-only were not being considered. Create a map with all of them and refactor transforms.isReadOnly() to streamline and efficiently check all uses of a certain value. Signed-off-by: L. Pereira <l.pereira@fastly.com>
This enables the optimization of functions such as "bytes.Equal()", which is currently implemented as: func Equal(a, b []byte) bool { return string(a) == string(b) } Which causes allocations and copies in TinyGo, but not in Big Go. Since we can now prove that neither conversions will escape, we can convert the calls to runtime.stringFromBytes() to an inlined version of it that doesn't allocate or copies the original string. Closes: tinygo-org#4045 Signed-off-by: L. Pereira <l.pereira@fastly.com>
0ce0e60
to
71f0663
Compare
Rebased to current |
Ping @aykevl |
I did a quick check and this doesn't seem to optimize the |
This optimization pass should have at least one test that shows that it actually does what it says that it does, and maybe a few tests that show that it doesn't apply in incorrect cases. I think this is actually pretty tricky to get right. For example, in the following case the optimization would be legal: func foo(b []byte) {
s := string(b)
return s[0]
} but in the following case, it would not be legal: func foo(b []byte) {
s := string(b)
bar() // might modify b
return s[0]
} |
(I'm sorry, but I won't be pursuing this optimization anymore as this was done as part of my previous job. If someone wants to pick this up, please feel free to.) |
@lpereira thanks for the update! That's unfortunate, but understandable. I'll close the PR to avoid clutter, but of course it can be reopened if you ever decided to continue work on it. |
This enables the optimization of functions such as "bytes.Equal()", which is currently implemented as:
Which causes allocations and copies in TinyGo, but not in Big Go.
Since we can prove that neither conversions will escape in this case, we can convert the calls to
runtime.stringFromBytes()
to an inlined version of it that doesn't allocate or copies the original string.This is a draft PR, because, although I can verify that the relevant calls to the runtime alloc calls are removed from the final binary in a simple test program,
make test
is failing. I'd like some help here to understand what's happening.Another thing that I didn't have time today to check was why
isReadOnly()
was returningfalse
for my test program; I'll pick this up on Monday to take a closer look, but for now, I'm omitting the call here, which is most likely whymake test
isn't happy.Closes: #4045
CC: @aykevl @dgryski