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.

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

Parallel lock free testing, remove potential deadlocks, cache static data, go to descriptor via test #3752

Merged
merged 20 commits into from
Jun 26, 2022
Merged

Parallel lock free testing, remove potential deadlocks, cache static data, go to descriptor via test #3752

merged 20 commits into from
Jun 26, 2022

Conversation

KvanTTT
Copy link
Member

@KvanTTT KvanTTT commented Jun 19, 2022 •
edited
Loading

@parrt I've fixed Java and C# runtimes. If it's ok, I'll unify other runtimes as well (where multithreading is actual)

Now Java runtime tests are superfast since they are inprocess (30s instead of 1.5 minutes)!

Also, now it's very easy to navigate to test data via tests in IntelliJ! See the video:

idea64_dBV9JpR0fg.mp4

@KvanTTT KvanTTT force-pushed the parallel-lock-free-testing branch from 8b757f6 to b0f4531 Compare June 19, 2022 18:43
@KvanTTT KvanTTT changed the title Parallel lock free testing, remove potential deadlocks, cache static data Parallel lock free testing, remove potential deadlocks, cache static data, go to descriptor via test Jun 20, 2022
@KvanTTT KvanTTT force-pushed the parallel-lock-free-testing branch from a2c8d68 to 17f9e9d Compare June 20, 2022 15:05
@parrt
Copy link
Member

parrt commented Jun 20, 2022

A huge amount of work here. thank you!

@KvanTTT
Copy link
Member Author

KvanTTT commented Jun 20, 2022

A huge amount of work here. thank you!

Thank you for your reviews and PR accepting :) I suggest changing other runtimes (moving EMPTY instances) to unify them with Java runtime (and probably to prevent potential deadlocks). I've complete it for Java and C#. It looks actual for C++, Swift., maybe Go.

@parrt
Copy link
Member

parrt commented Jun 20, 2022

Are we sure the others have deadlock issues? Not sure I have the energy to update. Maybe we can ping those target folks.

@KvanTTT
Copy link
Member Author

KvanTTT commented Jun 20, 2022 •
edited
Loading

Are we sure the others have deadlock issues? Not sure I have the energy to update. Maybe we can ping those target folks.

Not sure (because they can use another initialization mechanizm) but it's not fully excluded (C# is very similar to Java). But I also like the idea of synchonized API across runtimes. I can do that.

@KvanTTT KvanTTT force-pushed the parallel-lock-free-testing branch from 38e7869 to fb6fee8 Compare June 21, 2022 20:19
@parrt
Copy link
Member

parrt commented Jun 24, 2022

I had a random thought: Is any user code relying on EMPTY? I don't think so from a quick check but we want to be careful we don't break any user code as a result of this.

@KvanTTT
Copy link
Member Author

KvanTTT commented Jun 25, 2022

I had a random thought: Is any user code relying on EMPTY? I don't think so from a quick check but we want to be careful we don't break any user code as a result of this.

Yes, we should be careful, but removing deadlocks is much more important than insignificant changing of API. And since we change Java code, code of other runtimes can be changed as well.

@KvanTTT
Copy link
Member Author

KvanTTT commented Jun 25, 2022

I've almost completed everything, but I also want to update documentation.

@KvanTTT
Fix potential deadlocks in Java runtime
4ccbb41 
Referencing subclass ParserRuleContext from superclass RuleContext initializer might lead to class loading deadlock

Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Unify C#, C++, Dart, Go, Python runtimes
dbce6b2 
Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Use AtomicInteger instead of int for static PredictionContext.globalN…
8861ac4 
…odeCount

Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Remove lock in Generator (now ANTLR tool is thread-safe)
3c30268 
Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Cache templates for codegen to static field, load only one time
7c5b3c2 
Remove useless loadTemplates overloads

Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Cache LeftRecursiveRules.stg to static field
f19cd67 
Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Cache ANTLR error message templates to static field
6681a7a 
Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Cache runtime test templates to static field
93970f4 
Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Clarify message of failed runtime tests
1b9d77c 
Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Pass testSourceUri to dynamicTest and to dynamicContainer for conveni…
99fab5c 
…ent navigation to tests data in IntelliJ

Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Extract prepareGrammars method
8e6e1ed 
Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Remove useless and outdated files
2e8e3a8 
Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT KvanTTT force-pushed the parallel-lock-free-testing branch from fb6fee8 to ff655e3 Compare June 25, 2022 22:18
@KvanTTT
Copy link
Member Author

KvanTTT commented Jun 25, 2022 •
edited
Loading

I think now it's ready for review and merge.

@KvanTTT KvanTTT marked this pull request as ready for review June 25, 2022 22:25
@KvanTTT KvanTTT force-pushed the parallel-lock-free-testing branch from ff655e3 to f235d33 Compare June 25, 2022 22:51
@parrt
Copy link
Member

parrt commented Jun 26, 2022

In the middle of the review but wanted to comment that this is very well structured...each commit does a logical unit of work. thanks! And amazing piece of refactoring overall 💯

parrt
parrt reviewed Jun 26, 2022
View reviewed changes
doc/antlr-project-testing.md Outdated
@@ -87,6 +87,8 @@ And the result of testing the entire subdirectory:

All test are run in parallel both via maven and via IDE.

In IntelliJ, it's very easy to go to source by right-click on any test and pressing `Jump to source` (F4).
Copy link
Member

Choose a reason for hiding this comment

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

"right-clicking"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@KvanTTT
Copy link
Member Author

KvanTTT commented Jun 26, 2022

In the middle of the review but wanted to comment that this is very well structured...each commit does a logical unit of work. thanks! And amazing piece of refactoring overall 💯

I'd like to thank my @JetBrains team. They taught me to keep structured git history and write very conscious code.

@parrt parrt added this to the 4.10.2 milestone Jun 26, 2022
@parrt
Copy link
Member

parrt commented Jun 26, 2022

An amazing PR if I ever saw one thanks!!! Merging...

@parrt parrt merged commit ff2acb1 into antlr:dev Jun 26, 2022
@KvanTTT KvanTTT deleted the parallel-lock-free-testing branch June 26, 2022 21:36
@KvanTTT KvanTTT mentioned this pull request Jun 27, 2022
Closed
lppedd added a commit to lppedd/antlr-kotlin that referenced this pull request Dec 5, 2023
@lppedd
refactor: fix potential deadlocks in Java runtime (antlr4#3752)
90ef7a5 
See antlr/antlr4#3752
Commit 875911c2b2e6e50317e75ff2b5e80f4014a75954
@admin-token-bot admin-token-bot mentioned this pull request Jul 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parrt parrt parrt left review comments

Assignees
No one assigned
Labels
comp:runtime comp:testing type:improvement
Projects
None yet
Milestone
4.11
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@KvanTTT @parrt