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

kernel: remove input stream stack #4122

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

fingolfin
Copy link
Member

Instead of pre-allocating a fixed number of TypInputFile instances on each
thread, we allocate the required storage dynamically on the stack. This
arguably makes it easier to reason about the global state of GAP.

It also enables future simplifications and improvements, e.g. further
decoupling the input file code from the line buffering and potential
optimizations resulting from that.

Next step: minimize or ideally eliminate usage of IO()->Input resp. GetCurrentInput.

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Oct 2, 2020
@fingolfin fingolfin force-pushed the mh/remove-input-stack branch 2 times, most recently from ab471e2 to e24bfda Compare October 2, 2020 12:45
@coveralls
Copy link

coveralls commented Oct 2, 2020

Coverage Status

Coverage decreased (-8.0e-06%) to 93.728% when pulling 47dbd66 on fingolfin:mh/remove-input-stack into c955ca0 on gap-system:master.

@ChrisJefferson
Copy link
Contributor

Maybe mention in the commit message we are removing the (apparently unused) ToggleEcho? I guess at some point someone wanted it -- no need to mention it in the release notes.

Instead of pre-allocating a fixed number of TypInputFile instances on each
thread, we allocate the required storage dynamically on the stack. This
arguably makes it easier to reason about the global state of GAP.

It also enables future simplifications and improvements, e.g. further
decoupling the input file code from the line buffering and potential
optimizations resulting from that.
@fingolfin
Copy link
Member Author

Aha, good point, I forgot about the removal of ToggleEcho -- actually, I could just not remove it, too (in an earlier version of this PR I was still trying to get rid of the complete global IO()->Input pointer, but I gave up on that when I realized that GAP code actively makes use of that). Well, and ToggleEcho was not used by anything as far as I could tell...

@fingolfin
Copy link
Member Author

I've added ToggleEcho back. Seems better to avoid potential conflicts of interest here; it could still be removed in another PR.

@fingolfin fingolfin merged commit 9723019 into gap-system:master Oct 12, 2020
@fingolfin fingolfin deleted the mh/remove-input-stack branch October 12, 2020 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants