kernel: move ReaderState out of GAPState and onto the stack #3819

fingolfin · 2020-01-01T00:17:49Z

This way, saving and restoring it becomes unnecessary. It also gets easier to reason about the behaviour of the code.

Note: the second commit in this PR should be removed before merging; it contains a gross #define hack which I added to make reviewing easier, by reducing the noise in which the actual changes are otherwise lost. It's still quite a bit, but hopefully manageable.

Continues work begun in PR #3805.

src/intrprtr.c

fingolfin · 2020-01-01T00:28:58Z

src/read.c


 struct ReaderState {

+ScannerState s;


The ScannerState becomes part of the ReaderState (I've intended to do it this way for quite some time now - finally it happens, yay!)

One way to make this perhaps slightly nicer, and avoid some of the diffs in this PR permanently (i.e. not just with my crappy HACK commit) would be to switch read.c to C++, and then make ReaderState into a "subclass" of ScannerState. This way, a ReaderState * can be passed to any function expecting a ScannerState.

But I have doubts whether that's enough to justify converting the whole file to C++... ?!

fingolfin · 2020-01-01T00:32:03Z

src/read.c

-    readTop     = rs->ReadTop;
-    readTilde   = rs->ReadTilde;
    tilde       = STATE(Tilde);
-    currLHSGVar = rs->CurrLHSGVar;


Here something "real" happens: we avoid the need to backing up parts of the ReaderState (and we also avoid questions about why we just save those parts of the Reader, but not others... Now we "save" all of them, automatically.)

fingolfin · 2020-01-01T00:33:49Z

src/read.c

-  UInt                currLHSGVar;
  UInt                userHasQuit;
  syJmp_buf           readJmpError;
  UInt                intrCoding;


Hmm, I guess we should rename SavedReaderState to something else...

Note that I am thinking about moving STATE(IntrCoding) and friends into a new InterpreterState (which ultimately also would be stack allocated...)

This was done in PR #3823 which also removed SavedReaderState

coveralls · 2020-01-01T01:49:36Z

Coverage decreased (-0.0005%) to 84.729% when pulling ece4b25 on fingolfin:mh/ReaderState into a4a3764 on gap-system:master.

fingolfin · 2020-01-07T16:28:11Z

src/read.c

-{
-    return (struct ReaderState *)StateSlotsAtOffset(ReaderStateOffset);
-}
+typedef struct ReaderState ReaderState;


Here we can see that there was a (per-thread) instance of struct ReaderState inside the global GAPState (accessed here via StateSlotsAtOffset), and now isn't anymore. ReaderState used to refer to a helper function to access it; now ReaderState is an alias for struct ReaderState, and we instead pass a stack-allocated instance of struct ReaderState around via function arguments

fingolfin · 2020-01-07T16:29:05Z

src/read.c

 **  be declared forward.
 */
-static void ReadExpr(ScannerState * s, TypSymbolSet follow, Char mode);
+static void ReadExpr(ReaderState * rs, TypSymbolSet follow, Char mode);


From here on, we see a lot of changes identical to this: instead of just a ScannerState, we now pass around a ReaderState (which contains a ScannerState).

fingolfin · 2020-01-07T16:30:14Z

src/read.c

-    GAP_ASSERT(ReaderState()->CurrentGlobalForLoopDepth);
-    ReaderState()->CurrentGlobalForLoopDepth--;
+    GAP_ASSERT(rs->CurrentGlobalForLoopDepth);
+    rs->CurrentGlobalForLoopDepth--;


We also see a lot of changes identical to this: instead of using ReaderState(), we now use a ReaderState instance named rs which the callers passes down to us.

fingolfin · 2020-01-07T16:30:44Z

src/read.c

    else if (s->Symbol == S_LPAREN) {
        Match(s, S_LPAREN, "(", S_COMMA | follow);
-        ReadExpr(s, follow, 'r');
+        ReadExpr(rs, follow, 'r');


Aaaand we also see a lot of these: instead of passing a ScannerState named s, we pass on a ReaderState called rs.

fingolfin · 2020-01-07T16:32:19Z

src/read.c



+// HACK
+#define s (&rs->s)


This HACK is part of the second commit (which is meant to be dropped before merging this), and only is here for review purposes, to reduce the number of meaningless diffs: where before we directly accessed a ScannerState named s, we now instead need to get the ScannerState from inside the ReaderState called rs. To hide this, we use this #define.

fingolfin · 2020-01-07T16:35:29Z

src/read.c

-    memset(s, 0, sizeof(ScannerState));
+    ReaderState reader;
+    ReaderState * volatile rs = &reader;
+    memset(rs, 0, sizeof(ReaderState));


Up to here, basically nothing happened besides the things I explicitly pointed out; 99% of the changes are change ScannerState * s -> ReaderState * rs and related changes.

Here now we allocate a ReaderState on the stack, and zero it.

Could we do ReaderState reader = {0} instead?

I actually tried this first, but it caused compiler warnings on Travis. I couldn't reproduce those on macOS or on my Ubuntu, probably because my versions of GCC were too new.

No problem. Stupid travis, but it's not worth adding code which will have warnings.

fingolfin · 2020-01-07T16:36:36Z

src/read.c

-    rs->ReadTop        = readTop;
-    rs->ReadTilde      = readTilde;
    STATE(Tilde)       = tilde;
-    rs->CurrLHSGVar    = currLHSGVar;


Real change: since we freshly allocated the ReaderState on the stack, there is no need to "restore" its content before returning.

fingolfin · 2020-01-07T16:36:45Z

src/read.c

-    rs->ReadTop      = readTop;
-    rs->ReadTilde    = readTilde;
    STATE(Tilde)     = tilde;
-    rs->CurrLHSGVar  = currLHSGVar;


Real change: since we freshly allocated the ReaderState on the stack, there is no need to "restore" its content before returning.

fingolfin · 2020-01-07T16:37:11Z

src/read.c

-    }
-
+    GAP_ASSERT(rs->LoopNesting == 0);
+    nr = ReadStats(rs, S_SEMICOLON | S_EOF);


Real change: since we freshly allocated the ReaderState on the stack, there is no need to "restore" its LoopNesting member here.

fingolfin · 2020-01-07T16:37:35Z

src/read.c

-    readTop     = rs->ReadTop;
-    readTilde   = rs->ReadTilde;
    tilde       = STATE(Tilde);
-    currLHSGVar = rs->CurrLHSGVar;


Real change: since we freshly allocated the ReaderState on the stack, there is no need to "preserve" its "previous" content.

fingolfin · 2020-01-07T16:38:07Z

src/read.c

-    memset(s, 0, sizeof(ScannerState));
+    ReaderState reader;
+    ReaderState * volatile rs = &reader;
+    memset(rs, 0, sizeof(ReaderState));


Real change: we allocate a ReaderState on the stack and zero it.

fingolfin · 2020-01-07T16:38:57Z

src/read.c

-  ReaderState()->StackNams   = NEW_PLIST( T_PLIST, 16 );
-  ReaderState()->ReadTop     = 0;
-  ReaderState()->ReadTilde   = 0;
-  ReaderState()->CurrLHSGVar = 0;


No need to preserve or restore these, as we stack allocate the ReaderState for each use freshly on the stack.

fingolfin · 2020-01-07T16:39:01Z

src/read.c

-  ReaderState()->StackNams   = s->stackNams;
-  ReaderState()->ReadTop     = s->readTop;
-  ReaderState()->ReadTilde   = s->readTilde;
-  ReaderState()->CurrLHSGVar = s->currLHSGVar;


No need to preserve or restore these, as we stack allocate the ReaderState for each use freshly on the stack.

fingolfin · 2020-01-07T16:39:35Z

src/read.c

    StructInitInfo *    module )
 {
-#if !defined(HPCGAP)
-    InitGlobalBag(&ReaderState()->StackNams, "src/read.c:StackNams");


StackNams now is in a ReaderState instance on the stack, and hence our stack scanning code ensure it is not garbage collected. So this line can go.

fingolfin · 2020-01-07T16:40:02Z

src/read.c

-    ReaderState()->ReadTop = 0;
-    ReaderState()->ReadTilde = 0;
-    ReaderState()->CurrLHSGVar = 0;
-    ReaderState()->CurrentGlobalForLoopDepth = 0;


There is no per-thread ReaderState as part of the GAPState anymore, so the above can go.

fingolfin · 2020-01-07T16:40:05Z

src/read.c

    .initKernel = InitKernel,

-    .moduleStateSize = sizeof(struct ReaderState),
-    .moduleStateOffsetPtr = &ReaderStateOffset,


There is no per-thread ReaderState as part of the GAPState anymore, so the above can go.

fingolfin · 2020-01-07T16:45:19Z

I've now left a bunch of comments that try to explain the changes in-depth. Perhaps that's enough to make some people dare to look at it, e.g. @DominikBernhardt @wucas @flyingapfopenguin @ChrisJefferson

ChrisJefferson

One comment, about removing memset and using {0} instead (which isn't required). There are a couple of places this change would be made.

wilfwilson

Thanks for the hand-holding comments Max.

In case you've not noticed, this PR now has conflicts with master.

This way, saving and restoring it becomes unnecessary. It also gets easier to reason about the behaviour of the code.

fingolfin added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jan 1, 2020

fingolfin requested review from ChrisJefferson, DominikBernhardt and wilfwilson January 1, 2020 00:17

fingolfin force-pushed the mh/ReaderState branch from 8e9f42e to 7ab4781 Compare January 1, 2020 00:18

fingolfin commented Jan 1, 2020

View reviewed changes

src/intrprtr.c Show resolved Hide resolved

fingolfin commented Jan 1, 2020

View reviewed changes

fingolfin force-pushed the mh/ReaderState branch from 7ab4781 to ef4f2b9 Compare January 1, 2020 22:36

fingolfin mentioned this pull request Jan 2, 2020

kernel: refactor interpreter state handling #3823

Merged

fingolfin force-pushed the mh/ReaderState branch from ef4f2b9 to 2013750 Compare January 6, 2020 22:30

fingolfin added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) and removed do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) labels Jan 6, 2020

fingolfin mentioned this pull request Jan 7, 2020

kernel: move Start/FinishAndCallFakeFuncExpr to intrprtr.c #3830

Merged

fingolfin force-pushed the mh/ReaderState branch from 2013750 to f4753f3 Compare January 7, 2020 12:10

fingolfin commented Jan 7, 2020

View reviewed changes

ChrisJefferson approved these changes Jan 8, 2020

View reviewed changes

wilfwilson approved these changes Jan 8, 2020

View reviewed changes

fingolfin force-pushed the mh/ReaderState branch from f4753f3 to d25867e Compare January 8, 2020 21:41

kernel: move ReaderState out of GAPState and onto the stack

ece4b25

This way, saving and restoring it becomes unnecessary. It also gets easier to reason about the behaviour of the code.

fingolfin force-pushed the mh/ReaderState branch from d25867e to ece4b25 Compare January 8, 2020 22:31

fingolfin merged commit acb61ab into gap-system:master Jan 9, 2020

fingolfin deleted the mh/ReaderState branch January 9, 2020 06:08

kernel: move ReaderState out of GAPState and onto the stack #3819

kernel: move ReaderState out of GAPState and onto the stack #3819

Uh oh!

Conversation

fingolfin commented Jan 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisJefferson Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fingolfin commented Jan 7, 2020

Uh oh!

ChrisJefferson left a comment

Choose a reason for hiding this comment

Uh oh!

wilfwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fingolfin commented Jan 1, 2020 •

edited

Loading

coveralls commented Jan 1, 2020 •

edited

Loading

ChrisJefferson Jan 8, 2020 •

edited

Loading