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

Span<T> #81

Open
richardschneider opened this issue Jun 13, 2019 · 6 comments
Open

Span<T> #81

richardschneider opened this issue Jun 13, 2019 · 6 comments

Comments

@richardschneider
Copy link
Owner

Let's add Span/Memory into the library. This is an enhancement, current methods stay the same, we just add new method overloads.

A proof-of-concept should be the MultiHash class.

@richardschneider
Copy link
Owner Author

@hanabi1224 are you interested in this?

@hanabi1224
Copy link
Contributor

@richardschneider Sure, and FYI, here's an interesting discussion around introducing Span to protobuf lib: protocolbuffers/protobuf#3431

@richardschneider
Copy link
Owner Author

Just for the record, here's a good article https://msdn.microsoft.com/en-us/magazine/mt814808.aspx?f=255&MSPPError=-2147217396

@richardschneider
Copy link
Owner Author

@hanabi1224 I noticed you submitted a PR to multiformats. This library is not used by net-ipfs-core. We have our own multiformats in this project.

@hanabi1224
Copy link
Contributor

hanabi1224 commented Jun 16, 2019

@richardschneider From what I understand, Span is about to allocate stack memory as appose to heap to reduce GC overhead thus get perf boost. It benefits scenaios that do heavy calculation / byte array munapulation the most(e.g. base58 encoding / decoding), I notice net-ipfs-core is using SimpleBase which does not have span support, then it's hard to get the benefit by using Span. FYI I have submitted PR to multibase to impelement Span version of base58 and benchmark shows good perf gain. If that is merged, maybe you can consider switching to that one (SimpleBase is external dependency anyway). Does that make sense?

@hanabi1224
Copy link
Contributor

hanabi1224 commented Jun 16, 2019

@richardschneider Also I notice that the current Multihash decoding logic leveages on CodedInputStream in protobuf which unfortunately does not support Span api, Regarding the most import function is GetVarInt, I found a good replacement in lib BinaryEncoding, I have helped the author release a version that supports Span api yesterday which should be much more performant than protobuf ReadInt32. To benefit from Span api, you may want to switch to that lib. (But I would not expect good perf gain here, it's mostly just a read function than doing calculation)

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

No branches or pull requests

2 participants