Avoid adding to closureBusy before all ATNConfig properties are set #1955
Conversation
Setting ATNConfig properties can change the hash code of the instance, leading to cases where the closureBusy set places objects in the wrong buckets. While this has not led to known cases of stack overflow, it has led to cases where one or more buckets contains a large number of duplicate objects, and the set's add operation goes from O(1) to O(n).
|
Sam ( @sharwell ), when we profile the new parser of Apache Groovy, we find
Here are grammar files of Apache Groovy: |
|
@danielsun1106 can youGive it a try now with latest master? |
|
@danielsun1106 I'll try to get a build published soon with this fix included. It didn't seem like it would have a big impact for your case, but it's a possibility. |
|
Which part of the API is different? |
|
I was on a very tight schedule, but helped @danielsun1106 set up an experiment related to this change. As I expected, this change wasn't a notable contributor to the scenario. From what I can tell, the number of users who benefit from this change is relatively small, but the impact for those cases can be substantial. I'll work with Daniel in the coming weeks to see if further optimizations can be made to the internals of the closure operation and/or improve the ways we help users refactor their grammar to improve performance when using the profiler. |
|
Excellent. thanks @sharwell As usual, you are the performance guru! |
|
@parrt Here is the commit to show the difference of API. |
Avoid adding to closureBusy before all ATNConfig properties are set. This fixes antlr#2372. This is a port of c8805ab from the Java runtime. That was PR antlr#1955.
Avoid adding to closureBusy before all ATNConfig properties are set. This fixes antlr#2372. This is a port of c8805ab from the Java runtime. That was PR antlr#1955.
Applied C++ changes for PR #1955.
Avoid adding to closureBusy before all ATNConfig properties are set. This fixes antlr#2372. This is a port of c8805ab from the Java runtime. That was PR antlr#1955.
Avoid adding to closureBusy before all ATNConfig properties are set. This fixes antlr#2372. This is a port of c8805ab from the Java runtime. That was PR antlr#1955.
Avoid adding to closureBusy before all ATNConfig properties are set. This fixes antlr#2372. This is a port of c8805ab from the Java runtime. That was PR antlr#1955.
Avoid adding to closureBusy before all ATNConfig properties are set. This fixes antlr#2372. This is a port of c8805ab from the Java runtime. That was PR antlr#1955.
|
This change was ported to the Swift runtime as part of PR #2407. |
Setting
ATNConfigproperties can change the hash code of the instance, leading to cases where theclosureBusyset places objects in the wrong buckets. While this has not led to known cases of stack overflow, it has led to cases where one or more buckets contains a large number of duplicate objects, and the set's add operation goes from O(1) to O(n).🔗 This bug was found during investigation of a report by @gkudva in #42 (comment).