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.

Sign up for GitHub

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

Jump to bottom

internal/zstd: refactoring literals.go implementation to add decodeSymbol method #64031

Open
aimuz opened this issue Nov 9, 2023 · 2 comments
Open

internal/zstd: refactoring literals.go implementation to add decodeSymbol method #64031

aimuz opened this issue Nov 9, 2023 · 2 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@aimuz
Copy link
Contributor

aimuz commented Nov 9, 2023

In the zstd implementation, readLiteralsOneStream and readLiteralsFourStreams duplicate a lot of code, and for the sake of code maintainability, I suggest implementing the duplicated code into a decodeSymbol method.

The duplicate code is available at https://github.com/golang/go/blob/master/src/internal/zstd/literals.go#L283-L326

fetchHuff := func(rbr *reverseBitReader) (uint16, error) {
	if !rbr.fetch(uint8(huffBits)) {
		return 0, rbr.makeError("literals Huffman stream out of bits")
	}
	idx := (rbr.bits >> (rbr.cnt - huffBits)) & huffMask
	return huffTable[idx], nil
}
t1, err := fetchHuff(&rbr1)
if err != nil {
	return nil, err
}
outbuf[out1] = byte(t1 >> 8)
out1++
rbr1.cnt -= uint32(t1 & 0xff)

This part of the code is used between readLiteralsOneStream and readLiteralsFourStreams. According to RFC 8878 3.1.1.3.1.6

Each of these 4 bitstreams is then decoded independently as a Huffman-coded stream, as described in Section 4.2.2.

The logic of their implementation is exactly the same, a decodeSymbol method can be implemented

func decodeSymbol(huffBits, huffMask uint32, huffTable []uint16, rbr *reverseBitReader, outbuf []byte, off int) (int, error) {
	if !rbr.fetch(uint8(huffBits)) {
		return 0, rbr.makeError("literals Huffman stream out of bits")
	}
	idx := (rbr.bits >> (rbr.cnt - huffBits)) & huffMask
	t := huffTable[idx]
	outbuf[off] = byte(t >> 8)
	off++
	rbr.cnt -= uint32(t & 0xff)
	return off, nil
}

The decodeSymbol can be implemented in readLiteralsFourStreams and readLiteralsOneStream using the simplified code
@aimuz aimuz added the Proposal label Nov 9, 2023
@gopherbot gopherbot added this to the Proposal milestone Nov 9, 2023
@Jorropo
Copy link
Member

Jorropo commented Nov 9, 2023

This is a private method in an internal package, I don't think needs to be a proposal. 🙂

@aimuz
Copy link
Contributor Author

aimuz commented Nov 9, 2023 •
edited

I don't think needs to be a proposal

Because I couldn't find the right category 🥲🥲

@bcmills bcmills changed the title proposal: internal/zstd: refactoring literals.go implementation to add decodeSymbol method internal/zstd: refactoring literals.go implementation to add decodeSymbol method Nov 9, 2023
@bcmills bcmills added compiler/runtime Issues related to the Go compiler and/or runtime. and removed Proposal labels Nov 9, 2023
@bcmills bcmills modified the milestones: Proposal, Backlog Nov 9, 2023
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 9, 2023
@mknyszek mknyszek modified the milestones: Backlog, Unplanned Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@mknyszek @bcmills @gopherbot @aimuz @Jorropo