Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(276)

Issue 141930043: code review 141930043: html: Change EscapeString to use a strings.Replacer, be...

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 4 months ago by kabloom
Modified:
7 years, 1 month ago
Reviewers:
CC:
golang-codereviews, rsc, bradfitz, r, gobot
Visibility:
Public.

Description

html: Change EscapeString to use a strings.Replacer, because it's hugely faster. 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

Patch Set 1 #

Patch Set 2 : code review 141930043: html: Change EscapeString to use a strings.Replacer, be... #

Patch Set 3 : code review 141930043: html: Change EscapeString to use a strings.Replacer, be... #

Patch Set 4 : code review 141930043: html: Change EscapeString to use a strings.Replacer, be... #

Patch Set 5 : diff -r ca2c8c76aed9 https://code.google.com/p/go/ #

Patch Set 6 : diff -r ca2c8c76aed9 https://code.google.com/p/go/ #

Patch Set 7 : diff -r ca2c8c76aed9 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -41 lines) Patch
M src/html/escape.go View 2 chunks +8 lines, -41 lines 0 comments Download

Messages

Total messages: 8
kabloom
Hello golang-codereviews@googlegroups.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
7 years, 4 months ago (2014-09-10 14:19:32 UTC) #1
kabloom
Hello golang-codereviews@googlegroups.com, rsc@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
7 years, 4 months ago (2014-09-10 17:20:52 UTC) #2
bradfitz
Seems fine to me, but we're past the Go 1.4 freeze, so this requires r@ ...
7 years, 4 months ago (2014-09-10 17:29:31 UTC) #3
kabloom
On 2014/09/10 17:29:31, bradfitz wrote: > Seems fine to me, but we're past the Go ...
7 years, 4 months ago (2014-09-10 17:36:48 UTC) #4
bradfitz
On Wed, Sep 10, 2014 at 10:36 AM, <kabloom@google.com> wrote: > On 2014/09/10 17:29:31, bradfitz ...
7 years, 4 months ago (2014-09-10 17:47:35 UTC) #5
kabloom
On 2014/09/10 17:47:35, bradfitz wrote: > On Wed, Sep 10, 2014 at 10:36 AM, <mailto:kabloom@google.com> ...
7 years, 4 months ago (2014-09-10 17:52:48 UTC) #6
gobot
R=golang-dev (assigned by bradfitz@golang.org)
7 years, 4 months ago (2014-09-29 19:30:30 UTC) #7
gobot
7 years, 1 month ago (2014-12-19 05:13:14 UTC) #8
R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/141930043 is best) in the description in your
new CL.

Thanks very much.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b