Skip to content

Conversation

ewanmellor
Copy link
Contributor

Remove HashMap, and replace all uses of it with dictionaries. There's
no need for us to have a custom HashMap implementation (mirroring the Java
standard library) when Swift's standard dictionaries work just fine.

Fix Parser.bypassAltsAtnCache. This was declared as a Parser instance
variable, when in the Java runtime it is static (and therefore the cache
outlives the Parser instances). It was also being handled in a
thread-unsafe manner, because the cache was being read outside of the
mutex that was supposed to be protecting it. Fix both issues by moving
the cache and the mutex so that they are static to the Parser module and
rewriting getATNWithBypassAlts.

Remove Parser.decisionToDFAMutex. The Java code uses a synchronized block
on ParserATNSimulator.decisionToDFA, but the translation to Swift had put
a mutex in Parser. The decisionToDFA value is shared between Parser,
ParserATNSimulator, and the generated parser, so a mutex in
ParserATNSimulator isn't blocking all possible accesses, so it's useless.
Since this is only code for debugging anyway, just remove the useless mutex
and simplify getDFAStrings and dumpDFA.

Remove HashMap, and replace all uses of it with dictionaries.  There's
no need for us to have a custom HashMap implementation (mirroring the Java
standard library) when Swift's standard dictionaries work just fine.

Fix Parser.bypassAltsAtnCache.  This was declared as a Parser instance
variable, when in the Java runtime it is static (and therefore the cache
outlives the Parser instances).  It was also being handled in a
thread-unsafe manner, because the cache was being read outside of the
mutex that was supposed to be protecting it.  Fix both issues by moving
the cache and the mutex so that they are static to the Parser module and
rewriting getATNWithBypassAlts.

Remove Parser.decisionToDFAMutex.  The Java code uses a synchronized block
on ParserATNSimulator.decisionToDFA, but the translation to Swift had put
a mutex in Parser.  The decisionToDFA value is shared between Parser,
ParserATNSimulator, and the generated parser, so a mutex in
ParserATNSimulator isn't blocking all possible accesses, so it's useless.
Since this is only code for debugging anyway, just remove the useless mutex
and simplify getDFAStrings and dumpDFA.
@mike-lischke
Copy link
Member

mike-lischke commented Dec 16, 2017

Ewan, when you use standard dicts how does the Swift target implement object equality? There's a big difference between just finding an object by hash or to ensure it's equal to another one? The algorithm makes heavy use of that everywhere. I don't see where object properties are compared actually.

Regarding the mutex: have you tested with multiple threads? The ANLTR4 tests don't have multithreaded parser tests, so failures in synchronization are not detected (had a case in the C++ runtime recently).

@ewanmellor
Copy link
Contributor Author

@mike-lischke Object equality is defined by func == overloads in Swift. For example, ATNState's looks like this:

public func ==(lhs: ATNState, rhs: ATNState) -> Bool {
    if lhs === rhs {
        return true
    }
    // are these states same object?
    return lhs.stateNumber == rhs.stateNumber

}

I haven't done any multithreaded testing myself, so if there's none in the test suite then it's not being covered. I made those changes by inspection. For example, it doesn't take much to figure out that this mutex isn't doing its job:

        var result = bypassAltsAtnCache[serializedAtn]
        bypassAltsAtnCacheMutex.synchronized { [unowned self] in
           ...
        }

@mike-lischke
Copy link
Member

Ah, right, forgot that these overloads are separate in Swift.

@parrt parrt added this to the 4.7.2 milestone Nov 8, 2018
@parrt parrt merged commit 0c63b68 into antlr:master Nov 8, 2018
@ewanmellor ewanmellor deleted the swift-remove-hashmap branch November 11, 2018 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants