It seems Weapon M is very prone to deadlocks, and I don't see a quick fix without restructuring the key classes to reduce coupling and synchronization.

The problem is you have two key threads - awt dispatch and network - fighting between the ScriptManager and DataParser. Often, network will have the DataParser lock then try to aquire ScriptManager to handle an event, however, awt dispatch has ScriptManager from activating a script, but that script needs to get state from the DataParser such as the current prompt.

What I'd love to do is get rid of all the synchronization and circular references everywhere and move to more clear boundries combined with lock-free data structures and patterns. Unfortunately, this would be a major reorg in the code.

Mongoose, what do you think?

Under what circumstances is that happening? When a script is activated, one thread (it could be either) enters a synchronized method of ScriptManager and queues the script initialization task to be performed by the AWT thread. When that task runs, the AWT thread does not have a lock on ScriptManager.

I had a script that in initScript(), accessed script.getCurrentPrompt() (or whatever that is, I don't have the code in front of me). Since the script executing was happening on the awt thread, it locked script manager but then wanted the lock for data parser to get the current prompt. However, the network thread had the data parser lock and wanted script manager to fire off a script event.

I just finished changing all event handling to use a unified event dispatcher, so it may be better now, but my workaround at the time was to just not do that I'd like to find a way to get rid of all the synchronization as it makes concurrency really hard to debug and hits performance hard, but that'll be a pretty large task I think.

I'm still not sure what you're saying. Script initialization is indeed performed in the awt thread, but it does not lock the script manager (or anything else) unless you do so explicitly.

Synchronization is required whenever two or more threads access the same data. It can't simply be gotten rid of. Fortunately, it's not the performance hit it was back in the early days of Java.

Well, it can be gotten rid of using lock-free data structures and immutability whenever possible. While it isn't as bad as it used to be, it still causes multicore systems to perform worse and it is a huge pain to debug/reason about. The industry is moving towards functional programming and immutability for good reason

Anyway, I backed out my script change that triggered it. Will give a thread dump next time I encounter it.

I'm thinking about how immutability would apply to complex classes like Sector. I suppose I could break out groups of related fields into their own classes. E.g., density, density warps, and density date would become an immutable DensityInfo. Sounds like something to do at the same time as replacing my crude serialization with a real DB.

Usually what I do is create builders for each data object, turning the data object into an immutable interface. The challenge is Java sucks for immutability, but it can be done. In this case, Sector should be an interface, only returning primitives or other interfaces w/o mutators. Then, something like Sector.builder() would create implementations and ensure everything returned is immutable. Some sort of service layer should be in place to isolate all mutative operations on those objects. Where this doesn't work is if an operation needs to span multiple objects, e.g. new stardock port that also modifies game settings, but I think we could live with that limitation. Now that the data model is more settled down, it might be a good idea to give it a go.

With the exception of sector notes, Sector is already immutable as far as anything outside its own package is concerned. My idea of putting groups of related fields into their own immutable classes doesn't solve the problem; the getters and setters of those instances would still have to be synchronized. The only way to completely eliminate synchronization in the JVM is if Sector were an immutable snapshot of a database record as it existed at a distinct point. And then the synchronization would still exist; it would just be pushed out to the DBMS and its own concurrency mechanisms.

No, even with your current persistence design, you wouldn't need any synchronization. First, eliminate mutators, including package-level stuff, and especially outside field access. Second, make objects like Sector an immutable interface and use a builder to construct new ones, using full copies for any mutation. Then, instead of using thread-unsafe HashMaps in Database, use copy-on-write maps. Then, you can snapshot the data for storage at any time, knowing you won't get corrupted data, and you won't need any synchronized blocks any longer.

All that said, I wouldn't say it is the most pressing need right now. I think basic functionality like default scripts, reports, and some built-in behaviors like bust prevention or escapes would be more valuable. Even architecturally, I'd put in something like Dagger first, getting rid of all those field accesses that make it way too easy to turn the internals into spaghetti soup

Then how is the "master" copy mutated?

Is this the pattern you're talking about?

Kinda. How I'd do it is this:

DataParser


sectors is a service/data access layer for all sectors, containing a copy-on-write array list, so that the update would happen atomically. Even better, you could throw the updated sector on a queue that is read by another thread every so often and serialized to disk. The builder here copies the properties of the sector into a new object and gives you a way to change its properties before the construction of the immutable object. No locks, synchronization, or even volatile fields necessary, and read performance is as fast as it comes.

A copy-on-write array list requires synchronization or volatile fields. Like I said, it has to happen somewhere. And I can't imagine it being very efficient to clone the entire sector array every time a change is made to any sector...

Sure, it uses a volatile field internally, but there are none in your code, which was my point. Furthermore, it is very cheap to do a copy of an array of references, so I'd be very, very surprised to see that as a bottleneck. Synchronization, on the other hand, on multi-core systems, is a huge bottleneck and one that is very difficult to track down and debug.