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

hash/maphash: Provide a purego implementation #47342

Closed
flimzy opened this issue Jul 22, 2021 · 14 comments
Closed

hash/maphash: Provide a purego implementation #47342

flimzy opened this issue Jul 22, 2021 · 14 comments

Comments

@flimzy
Copy link
Contributor

flimzy commented Jul 22, 2021

What version of Go are you using (go version)?

1.17rc1

What operating system and processor architecture are you using (go env)?

GopherJS with Go 1.17

What did you do?

64d323f improves the performance of the hash/maphash package, but in doing so makes the package even "less" safe than it was before. In 1.16 this was worked around in GopherJS (where pointer arithmetic is not possible) with gopherjs/gopherjs@ccebfda. The recent changes will require a much larger delta, as now every caller to rthash will need to be updated, to use calling semantics more similar to the 1.16 version.

What did you expect hope to see?

A purego implementation of hash/maphash would solve this problem. And perhaps it would be useful in other scenarios as well?

@gopherbot gopherbot added this to the Proposal milestone Jul 22, 2021
@flimzy
Copy link
Contributor Author

flimzy commented Jul 22, 2021

cc @dmitshur

@flimzy flimzy changed the title proposal: hash/maphash: Provide a purego implementation of rthash proposal: hash/maphash: Provide a purego implementation Jul 22, 2021
@mvdan
Copy link
Member

mvdan commented Jul 22, 2021

My initial reaction was "I thought purego was for avoiding asm and cgo, not unsafe", but I'm wrong: #23172 (comment)

@randall77
Copy link
Contributor

Are you proposing to replace hash/maphash with a purego implementation? What implementation would that be?
unsafe is used in that package for performance reasons. Any different implementation would probably need to be equally fast.

@flimzy
Copy link
Contributor Author

flimzy commented Jul 22, 2021

Are you proposing to replace hash/maphash with a purego implementation?

No. I would keep the existing one for performance where useable, and add a purego version behind build tags.

What implementation would that be?

TBD. But probably logically equivalent to what GopherJS is doing now (i.e. a slight variation from the Go 1.16 version).

@randall77
Copy link
Contributor

So far the only purego tags in the main repo are crypto libraries. I think there's a higher level decision to be made as to how much we want to sprinkle the standard library with purego backstops. For instance, we'd probably need a purego builder to test that the code works.
But other than that (big) concern, it sounds reasonable to me.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jul 22, 2021
@rsc
Copy link
Contributor

rsc commented Sep 1, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Sep 1, 2021
@rsc
Copy link
Contributor

rsc commented Sep 8, 2021

In net and os/user we have netgo and osusergo tags instead of purego.
The only purego I see is in some crypto packages.
However, it looks like we accepted the idea of purego and did not get around to the documentation of it: #23172.

Given that, it seems fine to have a purego hash/maphash.

@cristaloleg
Copy link

Not sure but maybe this should be accepted first? #23172

@randall77
Copy link
Contributor

#23172 is accepted. It just needs to be done.

@rsc
Copy link
Contributor

rsc commented Sep 15, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Sep 15, 2021
flimzy added a commit to gopherjs/gopherjs that referenced this issue Sep 17, 2021
This pulls in a few select functions from Go 1.16, which were optimized and thus made unsafe in Go 1.17.

golang/go#47342 should render this change obsolete if/when implemented.
flimzy added a commit to gopherjs/gopherjs that referenced this issue Sep 17, 2021
This pulls in a few select functions from Go 1.16, which were optimized and thus made unsafe in Go 1.17.

golang/go#47342 should render this change obsolete if/when implemented.
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Sep 22, 2021
@rsc
Copy link
Contributor

rsc commented Sep 22, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: hash/maphash: Provide a purego implementation hash/maphash: Provide a purego implementation Sep 22, 2021
@rsc rsc modified the milestones: Proposal, Backlog Sep 22, 2021
@nevkontakte
Copy link
Contributor

I would like to contribute to the implementation, but I don't think I would be able to help much with testing infrastructure concerns @randall77 mentioned in #47342 (comment).

One alternative I can think of is that both optimized and purego implementations are included in the default build as private functions, and build constraints only select which one is actually used. In that case it would be possible to add a test that verifies that both implementation behave exactly the same. Dead code elimination should be able to strip out one of the implementations in the build.

The drawback of this option would be that the code might get a bit less intuitive, and it's not an approach that's easy to generalize other packages we might want a purego version of.

@gopherbot
Copy link

Change https://go.dev/cl/468795 mentions this issue: hash/maphash: add purego implementation

@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Feb 27, 2023
@shehackedyou
Copy link

This ultimately will make a novice programmers try to use binaries from system commands; and instead will be able to build proper libraries around ABIs from C libraries; this project may have a specific scope for you but overall this is pretty massive for the Go community; it will make it both easier and faster to use C tools which was becoming a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

9 participants