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

html: EscapeString could be made faster using a strings.Replacer #8697

Closed
kbloom opened this issue Sep 10, 2014 · 5 comments
Closed

html: EscapeString could be made faster using a strings.Replacer #8697

kbloom opened this issue Sep 10, 2014 · 5 comments
Milestone

Comments

@kbloom
Copy link

@kbloom kbloom commented Sep 10, 2014

Consider the following benchmark:

package escaping

import (
        "html"
        "strings"
        "testing"
)

const noHTMLString = `Can you hear the people sing, singing the song of
angry men. It is the music of a people who will not be slaves again.`
const withHTMLString = `We're going to demand <strong>one million
dollars</strong> or we're going to threaten Earth with a "Laser".`

var escaper = strings.NewReplacer(
        `&`, "&amp;",
        `'`, "&#39;",
        `<`, "&lt;",
        `>`, "&gt;",
        `"`, "&#34;",
)

func BenchmarkHtml_EscapeString_nochanges(b *testing.B) {
        for i := 0; i < b.N; i++ {
                html.EscapeString(noHTMLString)
        }
}

func BenchmarkStrings_Replacer_nochanges(b *testing.B) {
        for i := 0; i < b.N; i++ {
                escaper.Replace(noHTMLString)
        }
}

func BenchmarkHtml_EscapeString_changes(b *testing.B) {
        for i := 0; i < b.N; i++ {
                html.EscapeString(withHTMLString)
        }
}

func BenchmarkStrings_Replacer_changes(b *testing.B) {
        for i := 0; i < b.N; i++ {
                escaper.Replace(withHTMLString)
        }
}

The performance is as follows:

BenchmarkHtml_EscapeString_nochanges      500000              3912 ns/op
BenchmarkStrings_Replacer_nochanges      5000000               368 ns/op
BenchmarkHtml_EscapeString_changes        500000              4597 ns/op
BenchmarkStrings_Replacer_changes        1000000              1360 ns/op


I've written https://golang.org/cl/141930043/ to change html.EscapeString so
that it uses a replacer internally, but it needs to wait until Go 1.5 development opens.
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 10, 2014

Comment 1:

Labels changed: added repo-main, release-go1.5.

Loading

@bradfitz bradfitz added this to the Go1.5 milestone Dec 16, 2014
@bradfitz bradfitz added this to the Go1.5 milestone Dec 16, 2014
@rsc rsc removed the repo-main label Apr 14, 2015
@josharian
Copy link
Contributor

@josharian josharian commented May 2, 2015

@kbloom do you plan to migrate your CL to Gerrit (the new codereview system)? If not, would you like one of us to do so for you? Thanks!

Loading

@dspezia
Copy link
Contributor

@dspezia dspezia commented May 8, 2015

I'm not sure the OP is still following this thread. I prepare a similar CL (crediting him, of course).

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 8, 2015

CL https://golang.org/cl/9808 mentions this issue.

Loading

@kbloom
Copy link
Author

@kbloom kbloom commented May 8, 2015

Thanks. I had lost track of your ping because it was in my personal email
rather than my corp email. (Plus the friction of getting set up on a new
review environment.) I can't even figure out how to LGTM your CL, but it
looks good to me.

On Fri, May 8, 2015 at 1:01 PM, GopherBot notifications@github.com wrote:

CL https://golang.org/cl/9808 mentions this issue.


Reply to this email directly or view it on GitHub
#8697 (comment).

Loading

@bradfitz bradfitz closed this in 2d9a50b May 8, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants