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

vlib: initial addition of x.encoding.asn1 #22783

Merged
merged 11 commits into from
Nov 10, 2024
Merged

Conversation

blackshirt
Copy link
Contributor

@blackshirt blackshirt commented Nov 7, 2024

This is a rather big addition to the experimental vlib namespace. This module is marked as an experimental, inspired by the go and some rust version of similar library, the exported api its rather big.
This PR includes the docs, (maybe getting updates in the next iteration) that describes some parts of the module.

The simple benchmark also included in the bench dir, with the result (in my gitpod account)
Regular benchmark produces this result:

(dev) $ v run bench/bench.v
Benchmarking ASN.1 encode...
Average example encode time: 13 µs
Benchmarking ASN.1 decode (with asn.decode)...
Average (asn1.decode) decode time: 3 µs
Benchmarking ASN.1 decode with Example.decode)...
Average (Example.decode) decode time: 2 µs

Built with -prod flag and rerun the bench

$ v -prod benchk/bench.v
gitpod /workspace/asn1 (dev) $ ./bench/bench
Benchmarking ASN.1 encode...
Average example encode time: 3 µs
Benchmarking ASN.1 decode (with asn.decode)...
Average (asn1.decode) decode time: 1 µs
Benchmarking ASN.1 decode with Example.decode)...
Average (Example.decode) decode time: 1 µs

The go version of benchmark produces following result:

$ go run bench/bench.go
Benchmarking golang Marshal...
Average Marshal time: 1 µs
Benchmarking Unmarshal...
Average Unmarshal time: 0 µs

The result with -prod flag is on par with the go result, even behind it.

Please, give its a review, feedback, look or tries.
Best regards,

Huly®: V_0.6-21229

@spytheman spytheman changed the title Initial addition of experimental encoding/asn1 module to the experimental vlib namespace vlib: initial addition of x.encoding.asn1 Nov 8, 2024
@spytheman
Copy link
Member

spytheman commented Nov 8, 2024

Can you please also convert some of the _test.v files from internal tests (doing module asn1 at their top), into normal tests, that do import x.encoding.asn1, and only use the public API of the module?

Having internal tests is convenient for testing the private parts of a module, but having ordinary tests, is also important, because they test the public API, and can also serve as additional examples of the usage, that are compiled and checked by the CI (the .v programs in the vlib/x/encoding/asn1/examples/ folder are not checked at all - as far as the CI is concerned they are just part of an examples module without tests, that is why it did not complain about doing import asn1 in them).

@blackshirt
Copy link
Contributor Author

Can you please also convert some of the _test.v files from internal tests (doing module asn1 at their top), into normal tests, that do import x.encoding.asn1, and only use the public API of the module?

Having internal tests is convenient for testing the private parts of a module, but having ordinary tests, is also important, because they test the public API, and can also serve as additional examples of the usage, that are compiled and checked by the CI (the .v programs in the vlib/x/encoding/asn1/examples/ folder are not checked at all - as far as the CI is concerned they are just part of an examples module without tests, that is why it did not complain about doing import asn1 in them).

Sure, i would add it at next iteration

@spytheman spytheman merged commit fdc49dc into vlang:master Nov 10, 2024
60 of 61 checks passed
@blackshirt
Copy link
Contributor Author

Thanks for the help, review and discussion . Thanks for merging it
@spytheman, @JalonSolov @Delta456

@blackshirt blackshirt deleted the asn1-x branch November 11, 2024 00:04
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.

4 participants