Skip to content

Hard-code global tables' contents instead of calculating them at run-time#131

Draft
adrianmay wants to merge 43 commits intotahoe-lafs:masterfrom
adrianmay:init-tables
Draft

Hard-code global tables' contents instead of calculating them at run-time#131
adrianmay wants to merge 43 commits intotahoe-lafs:masterfrom
adrianmay:init-tables

Conversation

@adrianmay
Copy link

@adrianmay adrianmay commented Mar 18, 2025

This PR is intended to be merged immediately after #130. On this PR page the present changes are muddled up with those of #130, so I recommend reading this diff instead:
adrianmay/zfec@remove-unsafePerformIO...adrianmay:zfec:init-tables

This PR completely removes fec_init and Haskell's initialize, supplying a C file that lists the numbers in the four global tables verbatim. (Tables that depend on n and k are unaffected.)

This sidesteps all the difficulties in making sure fec_init gets run once (especially in Haskell) and improves performance both by cutting out the run-time calculations and by allowing the 64k table to be loaded into RAM on demand and discarded when not in use.

Since #130's version bump was unavoidable, bumping again immediately (because fec_init disappears) seems better than the tech-debt of retaining a no-op version of fec_init.

tables.c has been generated by a program in write.c which includes fec.c. The program populates the tables using the established logic, and then writes their contents to stdout as initialised arrays in C syntax. A new makefile touches tables.c (because fec.c includes it) and runs the program with its stdout piped to write.c. This has been done and the resulting tables.c committed to source control, so this process is not a pre-build step, rather, write.c is just retained in source control for reference. Code and data that's used purely for table generation has been moved to write.c so it won't bloat the library.

Comment on lines +4 to +6
tables.c: write.c fec.c fec.h Makefile
touch tables.c
gcc -o write write.c && ./write > tables.c
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

tox.ini Outdated
@@ -1,5 +1,5 @@
[tox]
envlist = py37,py38,pypy3
envlist = py37,py38,py310,pypy3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no 39?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only did that so I could run the tests without downgrading my python. It's off topic really. Maybe there should be another ticket about supporting lots of python versions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, 3.7 and 3.8 are already EOL and 3.9 is done in October so all of those should go.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR or in a general CI-cleanup one?
#129 would seem a bit more relevant perhaps.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI has current configs (and doesn't use tox).

Seems this is merely developer tooling that has been sitting on the shelf for a while?

Either don't touch or fix I guess?

@hacklschorsch
Copy link
Member

hacklschorsch commented Mar 20, 2025

We'd like to see what CI has to say about this PR and #130

This workflow is awaiting approval from a maintainer

Asking @sajith since you were around recently <3 - can approve the CI run for this?

@adrianmay
Copy link
Author

Temporarily putting this in draft while we investigate why tables.c doesn't seem to make it through the whole build system.

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