Skip to content

[Swift] Changes to Mutex-es to guard the staticly shared [DFA] #3276

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 28, 2021
Merged

[Swift] Changes to Mutex-es to guard the staticly shared [DFA] #3276

merged 4 commits into from
Sep 28, 2021

Conversation

martinvw
Copy link
Contributor

Request for review: @janyou, @ewanmellor, @hanjoes, @rmehta33

By sharing the mutex in the same way as the DFA array it becomes possible to run in parallel

Fixes #3271

@martinvw
Changes to Mutex-es to guard the staticly shared [DFA]
00cf357 
By sharing the mutex in the same way as the DFA array it becomes possible to run in parallel

Added a test in ThreadingTests

Fixes: 3271
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
@martinvw martinvw mentioned this pull request Sep 14, 2021
Closed
2 tasks
@martinvw martinvw changed the title Changes to Mutex-es to guard the staticly shared [DFA] [Swift] Changes to Mutex-es to guard the staticly shared [DFA] Sep 14, 2021
@martinvw martinvw mentioned this pull request Sep 25, 2021
Closed
@ericvergnaud
Copy link
Contributor

Hi,

this sounds useful but I'm not sure your test is 'hungry' enough to prove that it works.
there is a genuine possibility that the first iteration ends before the second one starts,
plus they both generate the same DFA enrichment so might be missing important scenarios.
May I suggest that you try parsing longer and different inputs in parallel such that the test does proves that it works in the problematic scenarios too ?

@martinvw
Copy link
Contributor Author

Without my changes this test always fails and with it always succeeds. I do believe we should aim for a bit more code coverage but I doubt that multiple or much more complex grammar is needed, however a longer text of course make sense.

I do not completely understand the second part of your comment, I might need a hint about that one.

Thanks for your time!

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Sep 25, 2021 via email

@martinvw
Copy link
Contributor Author

martinvw commented Sep 25, 2021 via email

@martinvw
Made the test more complex to have different paths
9f07205 
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
@martinvw
Copy link
Contributor Author

martinvw commented Sep 27, 2021 •
edited
Loading

@ericvergnaud I updated the test with a bit more complex expressions, do you think this would be enough:

antlr4/runtime/Swift/Tests/Antlr4Tests/ThreadingTests.swift

Lines 16 to 38 in 9f07205

func testParallelExecution() throws {
let input = [
"2 * 8 - 4",
"2 + 8 / 4",
"2 - 8 - 4",
"2 * 8 * 4",
"2 / 8 / 4",
"2 + 8 + 4",
"890",
]
let expectation = expectation(description: "Waiting on async-task")
expectation.expectedFulfillmentCount = 77
for i in 0...77 {
DispatchQueue.global().async {
let lexer = ThreadingLexer(ANTLRInputStream(input[i % 7]))
let tokenStream = CommonTokenStream(lexer)
let parser = try? ThreadingParser(tokenStream)
let _ = try? parser?.s()
expectation.fulfill()
}
}

I checked the call trace and I see that we actually touch all the changed files with the current test, so that looks good.

EDIT: I checked instead of check, so it is already done.

@martinvw
Added on more part under the same mutex
7bd1d16 
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
@martinvw
Fix count for test
1a91833 
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
@ericvergnaud
Copy link
Contributor

@parrt blessed, the failing test is unrelated and is fixed through another PR

@parrt parrt merged commit 886d244 into antlr:master Sep 28, 2021
@martinvw
Copy link
Contributor Author

Thanks!

@parrt parrt added this to the 4.9.3 milestone Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
target:swift
Projects
None yet
Milestone
4.9.3
Development

Successfully merging this pull request may close these issues.

Swift Target Crashes with Multi-Threading
3 participants
@martinvw @ericvergnaud @parrt