Bytewise crc32 #41

k-danil · 2022-09-05T08:04:17Z

Good day!

This pull request introduce bytewise crc32 calculation instead of current bitwise. Code based on VLC implementation, but instead per-object table generation, it use static crc32 table (+1kb additional lifetime memory use but no recalculations and reallocations) .
Current:

Benchmark_updateCRC32/Calc_PAT_crc32
Benchmark_updateCRC32/Calc_PAT_crc32-8         	 9470034	       122.9 ns/op	       0 B/op	       0 allocs/op
Benchmark_updateCRC32/Calc_PMT_crc32
Benchmark_updateCRC32/Calc_PMT_crc32-8         	 4010506	       283.4 ns/op	       0 B/op	       0 allocs/op

Introduced in this patch:

Benchmark_updateCRC32/Calc_PAT_crc32_precomputed
Benchmark_updateCRC32/Calc_PAT_crc32_precomputed-8         	71367823	        16.79 ns/op	       0 B/op	       0 allocs/op
Benchmark_updateCRC32/Calc_PMT_crc32_precomputed
Benchmark_updateCRC32/Calc_PMT_crc32_precomputed-8         	18918052	        54.75 ns/op	       0 B/op	       0 allocs/op

Test's and benchmarks based on real TS headers. Old implementation renamed and kept in file for comparison sake, you can remove it safely.

asticode

Nice PR!

I'd rather see it organized differently, I've made comments.

asticode · 2022-09-07T09:15:37Z

crc32.go

+*/
+// https://github.com/videolan/vlc/blob/master/modules/mux/mpeg/ps.c
+
+var tableCRC32 = [256]uint32{


Could you create a command to generate this table in a separate go file?

Add an internal/cmd/crc32_table folder

Add a main.go in it that would create a crc32_table.go with only the tableCRC32 variable in it.

Also make sure to add // Code generated by astits. DO NOT EDIT. a the top of this file before the package name

That way we have a command to generate the file

All done! This is my first attempt with code generation, so it may be kind of ugly

asticode · 2022-09-07T09:16:27Z

crc32.go

+
+// computeCRC32 computes a CRC32
+// https://stackoverflow.com/questions/35034042/how-to-calculate-crc32-in-psi-si-packet
+func computeCRC32old(bs []byte) uint32 {


You can remove the old implementation

asticode · 2022-09-07T09:17:30Z

crc32_test.go

+	},
+}
+
+func Benchmark_updateCRC32(b *testing.B) {


No need to keep the benchmark here

asticode · 2022-09-07T09:17:37Z

crc32_test.go

+
+func Test_updateCRC32(t *testing.T) {
+
+	for _, test := range tests {


You can remove this

asticode · 2022-09-07T09:18:35Z

crc32_test.go

+	}
+
+	for _, test := range tests {
+		t.Run(test.name+" precomputed", func(t *testing.T) {


You can remove the " precomputed" here

…lation func and corresponding tests/benchmarks.

asticode · 2022-09-08T07:19:27Z

crc32.go

@@ -4,22 +4,17 @@ const (
 	crc32Polynomial = uint32(0xffffffff)
 )

-// computeCRC32 computes a CRC32
-// https://stackoverflow.com/questions/35034042/how-to-calculate-crc32-in-psi-si-packet
+// this solution based on vlc implementation + static crc table (1kb additional memory on start, without reallocations)


Could you put this comment above updateCRC32 instead ?

And could you replace it with :

// Based on VLC implementation using a static CRC table (1kb additional memory on start, without // reallocations): https://github.com/videolan/vlc/blob/master/modules/mux/mpeg/ps.c

asticode · 2022-09-08T07:20:23Z

internal/cmd/crc32_table/main.go

+)
+
+const (
+	disclaimer = `// Code generated by astits. DO NOT EDIT`


Could you update the disclaimer to // Code generated by astits using internal/cmd/crc32_table. DO NOT EDIT

asticode · 2022-09-08T07:21:02Z

internal/cmd/crc32_table/main.go

+)
+
+func main() {
+	file, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|os.O_SYNC, 0644)


Could you use os.Create() instead ?

Yeah, this approach is much neater, thanks!

asticode · 2022-09-08T07:21:18Z

internal/cmd/crc32_table/main.go

+func main() {
+	file, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|os.O_SYNC, 0644)
+	if err != nil {
+		panic(err)


Could you use log.Fatal(err) instead ?

asticode · 2022-09-08T07:22:23Z

internal/cmd/crc32_table/main.go

+	doOrPanic(fmt.Fprintf(w, "\n%s", `}`))
+}
+
+func doOrPanic(_ int, err error) {


Could you rename this to write() and instead of panic(err) do log.Fatal(err) ?

On my opinion it is more general purpose function then just writer (and it's not even write, just crash if something go wrong). Maybe 'try()' will be more appropriate name? Anyway, i done as you ask

asticode · 2022-09-08T12:01:47Z

fyi I've created a v1.11.0 tag

thanks for the PR! ❤️

Bytewise crc32

e432377

asticode requested changes Sep 7, 2022

View reviewed changes

Bump go-astikit to 0.30. Make crc32 generator. Remove old crc32 calcu…

92fe9e2

…lation func and corresponding tests/benchmarks.

asticode requested changes Sep 8, 2022

View reviewed changes

Replace OpenFile with Create in crc32 generator. Some minor changes

90bb17d

asticode merged commit b2efeb7 into asticode:master Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bytewise crc32 #41

Bytewise crc32 #41

k-danil commented Sep 5, 2022 •

edited

Loading

asticode left a comment

asticode Sep 7, 2022

k-danil Sep 7, 2022

asticode Sep 7, 2022

k-danil Sep 7, 2022

asticode Sep 7, 2022

k-danil Sep 7, 2022

asticode Sep 7, 2022

k-danil Sep 7, 2022

asticode Sep 7, 2022

k-danil Sep 7, 2022

asticode Sep 8, 2022

k-danil Sep 8, 2022

asticode Sep 8, 2022

k-danil Sep 8, 2022

asticode Sep 8, 2022

k-danil Sep 8, 2022

asticode Sep 8, 2022

k-danil Sep 8, 2022

asticode Sep 8, 2022

k-danil Sep 8, 2022

asticode commented Sep 8, 2022


		func Test_updateCRC32(t *testing.T) {

		for _, test := range tests {

Bytewise crc32 #41

Bytewise crc32 #41

Conversation

k-danil commented Sep 5, 2022 • edited Loading

asticode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asticode commented Sep 8, 2022

k-danil commented Sep 5, 2022 •

edited

Loading