Skip to content

Thread safe whitespace#50

Open
david-davies wants to merge 25 commits intomainfrom
thread-safe-whitespace
Open

Thread safe whitespace#50
david-davies wants to merge 25 commits intomainfrom
thread-safe-whitespace

Conversation

@david-davies
Copy link
Copy Markdown
Collaborator

Closes #49

Re-implement mkSpace using an IORef to a map of threads to their white space parsers.

@david-davies david-davies requested a review from j-mie6 November 10, 2024 19:10
@david-davies david-davies marked this pull request as ready for review November 15, 2024 20:14
@david-davies david-davies added the enhancement New feature or request label Nov 15, 2024
atomically replace the WkThreadMap with `aliveThreads` (given by partition).
However, if we are pre-empted, and wake up later, `aliveThreads` may contain a thread that died while
we were asleep. In this case, we will have mistakenly added a dead thread back into the list.
-}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Eh, partition is still good: we will not "put back alive threads" into the map, just into the weak-thread map. We might miss a dead-thread who died during the alive list, but that's ok, the next thread along will pick them up. We can't be perfect here, as threads don't clean themselves up from the structure. We are just aiming to ensure some amount of fairness.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ah right, you can clobber the threads who have joined the list since the partition...

@david-davies
Copy link
Copy Markdown
Collaborator Author

Strange bug atm.
In examples/ExprLang.hs, if I run parseProg "return (1 + 2)", the parser succeeds.
However, even though the program parser is defined in terms of expr, running parseExpr "(1 + 2)" fails (a now customised message) with the message:
Exception: GigaParsec.Internal.Token.Space.getMapRef: entry in `RefMap` not initialised for thread: 54

Somehow, the whitespace parsers are not being initialised in expr, but they are in program.
Should note that program is defined in terms of fully, which uses the lexical description, so perhaps that forces the initialisation.
Whereas expr is a precedence table that doesn't make an explicit reference to a lexical description.

@j-mie6
Copy link
Copy Markdown
Owner

j-mie6 commented Nov 21, 2024

Yes, without an explicit initSpace (or whatever it is), you need fully.

@david-davies
Copy link
Copy Markdown
Collaborator Author

Ahh right. Perhaps then we should make this explicit in the docs? (I don't think it is already)
Or is there a way to automatically ensure initSpace is called?

@j-mie6
Copy link
Copy Markdown
Owner

j-mie6 commented Nov 23, 2024

Can't ensure it, but the wiki docs would point this out, see here

@david-davies david-davies added bug Something isn't working minor This requires a PVP minor bump and removed enhancement New feature or request labels Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working minor This requires a PVP minor bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mkSpace is not thread-safe.

2 participants