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

First parameter of galDivide is always 1 #253

Closed
cristaloleg opened this issue Sep 26, 2023 · 9 comments
Closed

First parameter of galDivide is always 1 #253

cristaloleg opened this issue Sep 26, 2023 · 9 comments

Comments

@cristaloleg
Copy link
Contributor

Hi, I was scrolling through the code and found that galDivide 1st param is always 1 (4 usages in code and 1 in tests).

func galDivide(a, b byte) byte {

This func doesn't look like a hot function, but maybe propagating a const might give a small benefit (smaller binary, caching, etc.). WDYT?

Thanks.

@klauspost
Copy link
Owner

Good observation. It simplifies down to something like this:

// galOneOver is the same as galDivide(1, a).
func galOneOver(a byte) byte {
	if a == 0 {
		panic("Argument 'divisor' is 0")
	}
	logResult := logTable[a]
	if logResult != 0 {
		logResult ^= 255
	}
	return expTable[logResult]
}

If it was super-hot we could eliminate the if with another table, but it doesn't seem worth it.

@cristaloleg
Copy link
Contributor Author

but it doesn't seem worth it.

Feel free to close the issue, thanks.

@klauspost
Copy link
Owner

@cristaloleg I am sending in a PR.. I was mainly saying "adding a separate table isn't worth it".

klauspost added a commit that referenced this issue Sep 27, 2023
As note by @cristaloleg in #253 the first parameter is always 1 in uses of `galDivide`.

Implement a `galOneOver` that uses this fact and replace `galDivide`.

Make `expTable` fixed size and cast to uint8 to avoid bounds check on use.
@klauspost
Copy link
Owner

#254

@cristaloleg
Copy link
Contributor Author

I was mainly saying "adding a separate table isn't worth it".

Ah, my bad.

@klauspost
Copy link
Owner

klauspost commented Sep 27, 2023

@cristaloleg Actually expTable[255] == expTable[0], so we don't need the "if" since 0^255 == 255

@cristaloleg
Copy link
Contributor Author

Nice! By the way, logResult := logTable[a] ^ 255 looks like a new table to me.

logTable values are in [0, 255) range, so we can store the result of logTable[a] ^ 255 instead, wdyt? (if I'm not missing something)

@klauspost
Copy link
Owner

The XOR is so fast that I don't really see any point in adding 256 bytes to binaries just for that.

klauspost added a commit that referenced this issue Sep 27, 2023
* Use faster division for matrix construction
* Remove needless 0 check.

As note by @cristaloleg in #253 the first parameter is always 1 in uses of `galDivide`.

Implement a `galOneOver` that uses this fact and replace `galDivide`.

Make `expTable` fixed size and cast to uint8 to avoid bounds check on use.
@klauspost
Copy link
Owner

#254 Merged

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