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: optimize EscapeString and UnescapeString with avx instructions #53410

Closed
tpaint opened this issue Jun 16, 2022 · 6 comments
Closed

html: optimize EscapeString and UnescapeString with avx instructions #53410

tpaint opened this issue Jun 16, 2022 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@tpaint
Copy link
Contributor

tpaint commented Jun 16, 2022

No description provided.

@tpaint
Copy link
Contributor Author

tpaint commented Jun 16, 2022

The performance of functions html.EscapeString and htmlUnescapeString in _amd64 can benefit from the use of avx instructions. We propose to implement avx-optimized versions of both. It can be shown that significant improvements are possible for the benchmarks EscapeNone, UnescapeNone, Unescape, UnescapeSparse, UnescapeDense, and Escape.

@seankhliao seankhliao changed the title avx performance optimizations for EscapeString and UnescapeString: html html: optimize EscapeString and UnescapeString with avx instructions Jun 16, 2022
@seankhliao seankhliao added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 16, 2022
@seankhliao seankhliao added this to the Unplanned milestone Jun 16, 2022
@seankhliao
Copy link
Member

Not sure if there are going to be people to maintain assembly code for that in the standard library.

@gopherbot
Copy link

Change https://go.dev/cl/412834 mentions this issue: html: improve EscapeString and UnescapeString performance using avx instructions on _amd64

@seankhliao
Copy link
Member

cc @nigeltao ?

@nigeltao
Copy link
Contributor

I'm not familiar with the official proposal process, so @ianlancetaylor

We propose to implement avx-optimized versions of both. It can be shown that significant improvements are possible

However, I am not in favor of the idea, based on https://go.dev/cl/412834 adding 2000 lines of assembly plus 5000 lines of data tables. Performance is just one of the goals of the source code in the go standard library (and its golang.org/x extended family). Another goal is maintainability (as @seankhliao said). Another goal is teachability - many people read the stdlib and golang.org/x to learn how to write idiomatic Go code.

Furthermore, Go is a memory-safe language, which is important to many of its users. There is a high bar for accepting patches that break that feature.

The CL description (and this issue) also does not provide any benchmark numbers. For argument's sake, even if it sped up html.Escape by 2x, if a web server program spends only 1% of its time inside html.Escape, the overall effect is a 100 / 99.5 = 1.005x speed-up, which is hard to get excited about given the costs outlined above.

You are obviously free to publish your own fastunsafehtmlescape package and other programmers can easily opt into using that if they want its trade-offs.

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

This proposal has been declined as infeasible.
— rsc for the proposal review group

@rsc rsc closed this as completed Apr 6, 2023
@golang golang locked and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

5 participants