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

Optimize []byte to string conversions #4289

Closed
wants to merge 3 commits into from

Conversation

lpereira
Copy link
Contributor

@lpereira lpereira commented Jun 8, 2024

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 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 returning false 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 why make test isn't happy.

Closes: #4045
CC: @aykevl @dgryski

Copy link
Member

@aykevl aykevl left a 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.

transform/allocs.go Outdated Show resolved Hide resolved
@lpereira
Copy link
Contributor Author

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 (make tinygo-test-wasi) seem to be working, but I don't have all the dependencies for things other than WASM building, so I'll wait for the CI to see if I need to fix something unexpected.

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>
@lpereira lpereira marked this pull request as ready for review July 23, 2024 22:38
@lpereira
Copy link
Contributor Author

Rebased to current dev and marked it as ready for review.

@dgryski
Copy link
Member

dgryski commented Aug 6, 2024

Ping @aykevl

@aykevl
Copy link
Member

aykevl commented Aug 8, 2024

I did a quick check and this doesn't seem to optimize the bytes.Equal case?

@aykevl
Copy link
Member

aykevl commented Aug 8, 2024

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]
}

@lpereira
Copy link
Contributor Author

lpereira commented Aug 8, 2024

(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.)

@aykevl
Copy link
Member

aykevl commented Aug 9, 2024

@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.

@aykevl aykevl closed this Aug 9, 2024
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.

3 participants