This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
I tried to implement issue #13352 but found that Mutex.java was so complicated and dense that I could not at all follow what it was doing. Seemed easier to rewrite it, which I did. MutexTest still passes, except for 4 tests which try to enter the write mutex (incl. postWriteRequest) from the read mutex, which is illegal. (New Mutex does not permit this transition.) API change is only to add canRead() and canWrite(). Would appreciate a review.
Created attachment 9581 [details] Possible patch
Created attachment 9582 [details] Complete new source code, for reference
I've made some more changes after talking with Yarda. 1. No longer allocates objects when entering mutex fresh. 2. Now when you enter the read lock from inside a write lock, it behaves almost like a plain read lock - canWrite() is false, enterWriteAccess() is forbidden, postReadRequest() runs immediately, etc. However other threads cannot enter the read lock (until you exit your outermost write lock). This mode is useful e.g. for firing changes. 3. postWriteRequest now possible again from inside a read mutex. Will run the runnable in a write lock when you exit the outermost read lock. In case this outermost read lock was actually inside a write lock or several, will run it as soon as all other readers have left. In case it was a pure read lock, will try to acquire the write lock (may be competing with other threads but this is unavoidable).
Created attachment 9591 [details] Revised Mutex source
Created attachment 9592 [details] Revised test diff; now just 2 tests disabled (READ -> WRITE) and one changed (WRITE -> READ -> postReadReq runs immed.)
Plan to also deprecate existing constructors of Mutex and introduce: public Mutex(int level); public Mutex(Privileged p, int level); // no public Mutex(Object lock, int level) - too dangerous where level is any integer other than Integer.MAX_INT, and EVENT has an implied level of Integer.MAX_INT. Semantics: if you hold a mutex with a defined level (read or write), you may not acquire a mutex (read or write) with an equal or greater defined level, though you may acquire a mutex (read or write) with a lesser defined level (or no defined level) according to the normal rules. Should prevent mutex <-> mutex deadlocks.
Implemented lock ordering but have not yet tested it.
Created attachment 9593 [details] Current state; moduleconfig=slim appears to work normally
1. checkEnter is likely to be implemented easily by ThreadLocal - for each thread just remember the lowest entered value. That might be faster. 2. implementation of readAccess(Runnable) is dangerous. Imagine what will be printed by the following code (M1, M2 are mutexes): class TwoLocks implements Runnable { int type; public void run () { switch (type) { case 0: type = 1; M1.readAccess (this); break; case 1: type = 2; M2.readAccess (this); break; case 2: sout ("M1: " + M1.canRead () + " M2: " + M2.canRead ()); } } } new TwoLocks ().run (); will it be true/true or false/true. In my opinion we do not want to introduce such non-determinism there...
Re. #1: just keeping an int level could work, assuming you changed the current level both in enter*Access and exit*Access. Good idea. Need to check how it will work with EVENT. Anyway though I may want to keep a stack of entered mutexes by thread to ensure that they are entered and exited in nested order. Such a stack will be about the same speed as the current threadsToMutexes map (assuming the number of mutexes held by one thread is small, which is very likely), but using a simple list is probably better than a WeakSet for memory consumption. Re. #2: I am not sure what the problem is here. The Javadoc says that it may be run asynchronously or not and you cannot depend on which. If you don't like that, don't use that method; use e.g. readAccess(Mutex.Action).
Created attachment 9611 [details] Revised Mutex.java
Created attachment 9612 [details] Revised test patch
Created attachment 9634 [details] Holds current locks when running r/wAccess(Runnable) asynch
Created attachment 9636 [details] Revised Mutex.java - when running r/wAccess(Runnable) asynch, sure to acquire same set of locks as are currently held
However now I see another possible trouble area involving ordered mutexes which it would be better to have solved properly. If you write this code: lower.enterReadAccess(); try { higher.readAccess(new Runnable() public void run() { // stuff } }); } finally { lower.exitReadAccess(); } you might expect that the marked code would be run asynch, since it cannot be run immediately. Currently it will just give an error. However generally it is not proper to use readAccess(Runnable) there since if run asynch it will try to keep the same set of locks it currently has, which would not make sense here - unless it reordered them, which could permit it to (asynch) acquire the higher, then the lower, then run stuff. On the other hand, it is reasonable to think that this: lower.enterReadAccess(); try { higher.postReadRequest(new Runnable() public void run() { // stuff } }); } finally { lower.exitReadAccess(); } would work as expected, running the specified code in a read lock on higher as soon as it is safe to do so (i.e. during lower.exitReadAccess(), unless some other locks are already held). I am not sure whether the current distinction between readAccess(Runnable) and postReadRequest (similarly writeAccess(Runnable) and postWriteRequest) makes any sense and is at all intuitive. For me, it would make sense for readAccess(Runnable) to always block, just like other overloads of the method, and postReadRequest to run the runnable at the earliest time when it would be legal (possibly blocking), and optionally to have some other methods to run stuff asynch. Unfortunately usage of Mutex.EVENT.readAccess is to sometimes go asynch, which makes no sense to me. Of course the behavior of the old Mutex was never well-specified in the Javadoc, so it is unclear what existing clients expect.
I am making a branch: cvs -d $CVSROOTHACK rtag -b -r BLD200304010100 mutex_32439 openide_nowww_1 core_nowww_1 performance_nowww_1
Examining old sources, EVENT behaved as follows: 1. *Access(Mutex.*Action) ran synch if in AWT, else waiting. 2. *Access(Runnable) ran synch if in AWT, else later. 3. post*Request(Runnable) ran asynch in AWT. I would rather change this to: 1. *Access(Mutex.*Action) as before. 2. *Access(Runnable) as in 1, i.e. synch if in AWT, else waiting. 3. post*Request(Runnable) synch if in AWT, else waiting for AWT as soon as all ordered mutexes are released (possibly now). I.e. never use EQ.invokeLater. Possibly introducing some new AWT_SYNCH constant for the revised behavior, to keep compatibility for old code, and deprecated AWT. Need to create code samples for typical use cases involving two nested systems, e.g. Filesystems + (old) Datasystems, or Filesystems + Filesystems Extensions (FileObject lookup portion), or Filesystems + Registry, involving bidirectional change firing etc. However it is not clear whether there *should* be any cases where two (public) mutexes are in some relationship to one another. FS + old DS will hopefully go away. FS-Ext might well just use the same lock as FS. Registry might have a public lock and treat use of the SFS as an implementation detail, meaning that non-Registry code should not be accessing it anyway, and/or might share a lock with FS. In that case lock ordering on Mutex is probably unnecessary, or a warning could be printed when any Mutex is acquired while holding another, or when EQ.invokeAndWait is called while holding any normal Mutex. TBD.
Probably FS + MDR (~ParserDB) is the best example I can think of where lock ordering and generally Mutex <-> Mutex interaction would be helpful in 4.0. Maybe there is something in Projects that I don't know about which has a complex enough structure to merit this.
Or (re. FS + MDR/ParserDB/JavaHier) there could be a single Mutex for all "data" inside NB. Then there would be no lock ordering problems. It would mean you could not e.g. expand a folder while some source was being parsed, until that file was done. In such a system you need a way to easily break up big tasks into digestible pieces where possible and/or to cancel or postpone a task running in a lock.
I've been just talking with David K. He suggest to add Mutex.Privileged.getMutex, so he can keep just reference to Priviledged and return to clients Priviledged.getMutex. The pointer to Mutex is there anyway, so there is not reason why not expose it.
Created attachment 10106 [details] Mutex.Privileged.getMutex() patch
What do you think about that Jesse? Does it make sense? I do not want to go into details but where I would use this is in Registry SPI where would be method Mutex.Priviledged getMutex(); and in API I would give API clients just Mutext by calling Mutex.Priviledged.getMutex(), but internally in the Registry impl I would take advantage of Mutex.Priviledged methods.
Yes, M.P.gM() makes perfect sense, I will add it. Thanks for the tip.
Jesse, what do you think about this: Merged context delegates to list of SPI contexts. Each of its delegates can have its own Mutex.Priviledged (ie. Mutex.Privileged RootContext.getMutex()). The merged context needs to return something like MergedMutexPrivileged[del1.getMutex(), ..., deln.getMutex()] where enterReadAccess calls enterReadAccess on all delegates, etc. Do you think something like that is reasonable? For some time I was thinking about restricting merged context to accept only contexts with the same Mutex (ie. RootContext). It could work for some time, but sooner or later somebody will create different implementation of context and might want to merge it into default context and then this won't be sufficient anymore - they will have their own mutexes. Passing everybody the same mutex is also not good - the context's mutex should be immutable. Implementing methods like enterRead() etc. seems to me quite easy. Methods like postReadRequest() etc. are a bit more complicated but still doable I think.
Re. Registry API and mutexes. 1. Please don't put Mutex.Privileged into the API. Some threading models, including the one that I currently find most promising, do *not* support anything like M.P - i.e. you cannot enter the mutex and then have control flow return to the caller, you really need to pass the entire runnable in order for it to work. 2. Keep it simple and assume that all the merged contexts use the same mutex. Hopefully in fact they will. Or keep it simpler and define *the* mutex to use, for any context, as a static constant. Why would you need more than one?
ad1: By API in this context you mean also SPI, right? M.P is at the moment in SPI.RootContext.getMutex(). Will not be difficult to change it because there is not (and will not be) many impls. ad2: I will think about it. At the moment it makes lot of sense to me that default registry and each project should have its own mutex. If they all should share the same mutex I would afraid of potential performance bottleneck.
>potential performance bottleneck Lock contention, you mean? On single-CPU box? Oh, come on!
IMO, individual projects registries and default system registry are totally independent parts which should not share one lock. But in reality it might not be an issue.
Re. #1 - right. Re. #2 - see Petr's reply.
Rather than trying to adjust the behavior of the existing Mutex class - which has (1) a misleading name, (2) poor support for switchable implementations, (3) some poorly specified methods like post*Request - I plan to create alternate lock interfaces in classes in performance/threaddemo/src/threaddemo/locking/ and use them just in the thread demo for now. If they seem to be useful enough to include in the main source base, they can be moved to openide-util.jar. Few places in the public APIs as of 3.5 refer directly to Mutex anyway; only Children, I think, and this probably should not need any locking anyway.
Good plan, at least there will be no regressions. Btw. Another place where mutex is in API is Compiler. If you have some tests that are passing with current impl of Mutex, please commit them to main test suite. Will the old mutex be rewritten to use the new interfaces sometime?
Re. Compilable.MUTEX - OK, but I don't much care about that API. I wonder if anyone outside the compilation system is even using that field. Re. new tests - there isn't much new that would apply to the "traditional" Mutex, i.e. a r/w lock, so it doesn't seem too useful to backport them. Re. adapting Mutex to new interfaces - if the new stuff goes into openide trunk, I could try to provide a method in Mutex: public Lock asLock(); which would serve as a bridge, or just make it impl the new interface. However the interfaces do not line up exactly. For example, old Mutex has no equivalent of canRead/canWrite, and I have no idea how to add it (because as I may have mentioned, I have no idea how the impl of old Mutex actually works). Anyway all this is still provisional - I have yet to see any demonstration that we actually need read/write locks for anything, i.e. that we could not just provide some static Object monitor to synchronize on for APIs that need to work with multiple threads. R/W locks are only useful if you have heavy usage of the data model in read mode from multiple simultaneous threads - on a multi-CPU machine, or on a 1-CPU machine with an aggressive thread scheduler with one of the threads being the event thread.
Probably best to not touch Mutex at all, just use JDK 5 concurrency tools, and think about when we can deprecate it. (Tough because of usage from both Children and ProjectManager.)