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

Remove fastpath and unsafe variants #14

Merged
merged 7 commits into from
Dec 5, 2022
Merged

Remove fastpath and unsafe variants #14

merged 7 commits into from
Dec 5, 2022

Conversation

swenson
Copy link

@swenson swenson commented Nov 30, 2022

The fastpath and unsafe variants rely on particular values for Go internals and a lot of unsafe assumptions about types, functions, strings, arrays, etc.

We are not committed to maintaining these dangerous code paths, so it is safer to remove them altogether, and rely on the "not unsafe" and slow paths.

This will make this package a bit slower, but also a much smaller dependency, since the fastpath generated code is extremely large.

@swenson swenson requested review from tomhjp and kisunji November 30, 2022 20:05
Copy link

@kisunji kisunji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could yout point me towards which files remove unsafe?
My understanding is that fastpath and unsafe are controlled by build flags; I see the fastpath being disabled in fast-path.not.go but unsure where to look for the unsafe

@swenson
Copy link
Author

swenson commented Dec 2, 2022

@kisunji helper_unsafe.go is where most of the unsafe code is. https://github.com/hashicorp/go-msgpack/pull/14/files#diff-9891b7f524743080430997fca4d9f041448a1d128e309d11e3768c317ee8068fL23 (this line is an example of the things that strike me as making these code paths hard to maintain)

It's worth noting that fastpath and unsafe are the defaults, so if you include go-msgpack as a dependency, you get the fastpath and unsafe versions.

@kisunji
Copy link

kisunji commented Dec 2, 2022

Ah I see it now, thanks. Will your refactors make it difficult to pull more upstream changes? Might not be a huge cost in the end.

@swenson
Copy link
Author

swenson commented Dec 2, 2022

Yes, this will make it hard to pull in upstream changes.

Really, at this point, we have effectively diverged with upstream, and this fork should focus more on safely supporting the current msgpack (and JSON for Nomad) formats, and drop everything else.

If folks want to have the newer features and performance of the upstream package, then they should use the upstream package. :)

@swenson swenson merged commit 948591c into main Dec 5, 2022
@swenson swenson deleted the remove-stuff branch December 5, 2022 18:53
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.

2 participants