Synchronization of TrackerAdapterWorker for probable future race conditions

[TPolzer]

TrackerAdapterWorker has quite a few race conditions. I do not know if this is worth the effort to fix, see #129.

[gayathery]

"quite a few race conditions" is abstract. If you come across a race condition where it is not working, please send me the sequence of steps to follow to reproduce the race condition and then i can look into it.

[TPolzer]

The point of a race condition is that is not reproducible easily ;) You do not even synchronize at all, so at some point after jit optimizations it could even happen that none of what TrackerAdapterWorker does is visible to other threads. This is not an abstract concern. Basically everything you do with the indexing is a race condition just waiting to happen!

[gayathery]

race conditions are not reproducible easily but can be described if it occurs or if you find a particular sequence it happens. just simply it cannot be claimed that there are race conditions. a bug cannot be raised without a concrete situation, either it should have occured or a proper scenario has to be analyzed. in the existing code there is a clear partition when TrackerAdapterWorker will be used and when it is not. just because there is threading in application it cannot be claimed to be a race condition. in fact in this logic snychronization for worker is not required, because when it finds a change in underlying data. it calmly hand over the control to the normal route until it is up to date. So i am closing this issue untill there is a reported concrete scenario

[TPolzer]

Simple scenario: isThreadingRequired is false, and isReadyForTakeOver = true; in prepareData() gets reordered from line 207 to line 120. I think you can figure out by yourself what happens now.

This is just one example, "quite a few" was really an understatement.

[gayathery]

in the current implementation isThreadingRequired is always false. and there are some changes to be done before i set it to true. so there is only one case the code will execute that is from the else part in line 172 and i do not see and reordering here. firstly reordering itself is an abstract term in case of threading...if you know some scenario...be specific...

[TPolzer]

It is obvious you have no idea how threading works, please fix the code or revert it.

[TPolzer]

If you want to know more, read https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.4.5

[gayathery]

we can create or fix a bug only if it occurs and cannot fix a big which has not yet occured.

We have made sufficient improvement in application performance and now as per the priority we need to work on functional requirements. We still have to fix bugs in core logic.

So can fix this bug if it really occurs. That time we can re-open .

[TPolzer]

Yes we can write code which does not have the obvious possibility of spontaneous explosion built in. And you really do not want to fix a threading bug only when it manifests itself, that is real pain, believe me.

[TPolzer]

This is issue is still not fixed, so why did you close it?

[TPolzer]

mistakenly tagged this in commit message, sorry

[td5r]

Will we use TrackerAdapterWorker in future? Currently I am moving from CommitFile to Path at GUI model. (With "I" I mean "we") As I see, TrackerAdapterWorker implements equal methods as TrackerAdapter.... this doubles the work, especially if TrackerAdapterWorker won't be used anymore. (should I adapt the code of TrackerAdapterWorker to the newer data types?)

But I am still asking myself, why do we keep increasing our code complexity, if the main concern of current performance issues is due to over complicated code and software. (We lost track of the overall functionality..) I am sure, that we can gain performance improvements, if we (e.g.) fix this problem with mulitple calling of same functions/methods ...

[gayathery]

Firstly i want to say is ,TrackerAdapterWorker will be used in future for multiple purposes. Cache implementation was good but it does not make sense to say that first clicks of requirement will not have expected performance and later clicks will be fine. I have not seen any application specifying like this. (for example Requirement ID 22 takes a long time for the first click) the repository what we are using is just a tiny one compared to what is there in reality in a company. So just by testing our requirements in our repository we cannot say our application is performing well. we cannot tell the user to select all requirements once before using the application. Indexing does the job for this. Now indexing takes only around 45 seconds after which application is fast. some more improvements to threading will be done to make is safer and faster. After indexing Traceability matrix takes exactly 4 seconds compared to 38 seconds (without caching....as i already mentioned user will not click all requirements before generating matrix). another future possibility is to persist this data structure and use it across sessions till the underlying data is not changed. we are testing our application with just 800 files and some 100 requirements which is far far too less compared to a real system in a company where there will be thousands of files and thousands of requirements and data being changed numerous time (and hence a big big blame information). So while caching is a solution for certain scenarios, indexing is required for other scenarios. this is not a new concept i had invented, it is there for a long long time in many applications.i have checked the time taken for different methods and also had analysed the load of this indexing on the real application, indexing is not creating any performance overhead and it is also running in a thread which is having lower priority than the application. One more thing is TrackerAdapterWorker is using the equals methods and not implementing it. when there is a change in underlying data structure then obviously there will be changes in the places where it is being used. so it is wise to adapt these changes instead of removing functionality

[gayathery]

@GnuTalux Do you need any help in adapting the "TrackerAdapterWorker "

[td5r]

maybe later I will need your helps. ^^

Currently I am just trying to change essential codes to new data types and ignoringTrackerAdapterWorker. Ifs this refactoring is done, I will adapt TrackerAdapterWorker... Otherwise its much harder.

[gayathery]

@GnuTalux okay,.if you need any assistance in this case, please let me know

[yadimon]

letz do some solution @gayathery just refactor the indexer. Make it like a proxy of ModelAdapter. Or let it call the Adapters methods. We need easy maintanable code. Nothing copypasted or not-ready-yet . its critical in this case. (btw probably all we need, is just to call getCommits.. for all reqs. Its enough for caching already. But ok, we can cache again over indexer on GUI level. But think about eclipse, maybe you need the indexer there too. So maybe it should be somewhere in backend, and check guava cache, maybe it will be easy to use it in the indexer)

@TPolzer fix the possible threading bugs, after Gayethery is ready.

is it ok as solution, or too much not needed work?

[TPolzer]

Honestly, I would be strongly in favor of scrapping the indexer entirely, because it needs more or less a rewrite anyway, and I just do not see the benefit of the added complexity.

[gayathery]

@yadimon the indexer is need in the swing application. For eclipse plugin the requirement is to compute for one file at a time. The possible improvement need at ide side is improving gitblame and its resources if it is possible. (As this is the root of all the performance issues)

Working on other aspects you mentioned.

[gayathery]

@yadimon one more thing to note is already the TrackerAdapterWorker is calling only TrackerAdapter calls until it is indexing and later it uses its own data structure and there is no place the implementation is repeated.

In general I do not understand why it is required to scrap a functionality which is not hindering existing implementation and there is no obvious problems in our application because of this.

I am still certain that our application lacks performance. Req 22 takes 7 seconds for the first time...think about real time scenarios. just testing the application in a tiny repository and claiming to be having performance is not practical. Think about a situation where user of a real system (With minimum of 100,000 files and 5000 requirements) want to get a traceability matrix. Does it make sense to ask the user to use all requirements once before generating traceability matrix.

A functionality can be put in repository in an incremental way if it is not affecting existing functionality and there is no need to revert a feature until it is really posing a problem to the application functionality. Agile by itself means incrementally developing and releasing a shorter version of the product in every cycle and slowly developing the actual product across multiple iterations. So as per the Agile process the indexing would stay in the repository because there is no problem to the way in which the existing application is working. secondly it can be chosen by user. thirdly it is an incremental development and indexing can be improved across sprint iterations.

[gayathery]

For a sample performance report by a requirement management software...see the link and the point shown in the image....we can no way get something like that if our core logic of finding blame information is taking so much time. We should fix these current problems first before solving others. This can be no way achieved by only using caching. Getting blame information is our core functionality and all caching and indexing are done to mask the performance problems in our blame logic. when some software is able to achieve less than 1 second for a requirement where there are 20000 requirements is it not true that our blame performance is bad because it takes many seconds for just a meager 100 requirements and 800 files.

And a thing to note is,...this is not the best software in the industry...and you can look at its performance https://www.tracecloud.com/GloreeJava2/jsp/WebSite/TCCompare.jsp

[gayathery]

https://github.com/uv48uson/amos-ss15-proj3/commit/0acac358c8fbd70e2852ff7df872f54bc03574eb

[TPolzer]

You really managed to get rid of the races, but now you introduced the possibility of deadlocks (Exception while locked) and slowdowns in the gui (lock held while gui wants to do something). I fixed all this for you, but as mentioned in 28d05944d5ac51b1020d48887a0055534ffa3ad5, our code base is still not thread safe. But this should probably be a separate issue.

[gayathery]

@TPolzer It is agreed in our team that we should not delete others work without any valid current problems or valid notifications (in my case both are not true). i know there are some more places to handle. i had used a generic lock just because my next implementation was about to handle specific handling for thread interruption and further synchronization. I commited to master because the existing state of the code will not pose immediate problems. but before that you are in a hurry and commiting your changes just quoting that there are some problems in my code. do you think all our commits do not have a problem? Isnt it we work comit by comit by improving on the existing code.

[gayathery]

@TPolzer YOU ARE VIOLATING OUR AGILE PROCESS. You removed all the possibilites of thread handling of indexing which I had implemented already and had the possibility to switch it on in the future. It is a surprise that effort is put to remove the existing code which has no current problems instead of putting the effort on open issues (example quality and perforamance of blame) which are more needed to our application. I honestly dont understand this.

[gayathery]

And also i had already checked time taken before and after my implementation and there is no delay because of the code that i checked in. if you do not believe, i have the time taken information before and after the code changes

[TPolzer]

Concerning the slowdowns: I said possibility, it depends very much on when you do what. And anybody with reasonable experience in concurrent programming will tell you that the code before was a huge problem. And no I did not remove the possibility to switch it on, its functionality is still the same.

And yes it was already a problem that the code was unmaintainable, just look at what @GnuTalux wrote.

[gayathery]

@TPolzer I really dont know whether you know what is caching and what is indexing..you have spoiled the indexing by using caching which is completely irrelevant to the concept I had followed. Any now it is claimed to be cleaning of the concept. Since it is spoiling the concept of indexing i have to revert your changes

As per your statement "I did not remove the possibility to switch it on" you did not understand the existing concept first of all, that is why you are not able to understand what i told. I had multi threading concept for indexing put inside but it was switched off, now you completely removed it ...first read the theories on indexing and caching and then you can attempt to work on it. There are many issues with your Blame logic and I wanted to give space to you to fix them and follow Agile process but you are spending effort on deleting code which you do not understand fully. And regarding maintainability of code, Its bit humorous to say that TrackerAdapterWorker code is unmaintainable. if you want to change something, it is obvious that users of that code should be changed and it is not meant as unmaintainable, dont you think that our overall architecture is unmaintainable and not the indexing part?

[gayathery]

Since the following comit had spoiled the indexing concept, I had reverted it https://github.com/uv48uson/amos-ss15-proj3/commit/33918853316039a2b97a1bf39243db24bbb5f9d5

[gayathery]

Only thing i liked in your last commit was using getheadId instead of commit count. all others really spoiled the indexing concept

[TPolzer]

"spoiled the concept"... you have no idea what you are talking about, it was doing the same thing.

[gayathery]

no, you are trying to cache data and also you removed multithreading concept which was there earlier. believe me, if the concept was not spoiled i would not have reverted it.

[TPolzer]

Just please ask somebody else (since you obviously do not believe me) with experience in this sort of thing to have a look at the current code.

[gayathery]

though the multithreading concept is not enabled earlier, the complete implementation was there, if need arises this can be switched on (isThreadingRequired) and indexing can become much faster. i had the implementation there and only disabled it to check the performance without that. I accept there is a need to implement the exceptoin part of the lock...i had a different thought about it.if you had only reworked for the deadlock situation instead of changing the complete concept i would have appreciated it

[gayathery]

@TPolzer just now i saw about the deadlock problem you had told. this deadlock will not happen in the existing application in the generic lock class because when there is an exception, the lock is no more locked and further unlock will not be a problem too. can you describe whether this was the deadlock situation you were speaking about?

[gayathery]

https://github.com/uv48uson/amos-ss15-proj3/commit/f35a894ee816eac57910ba3cfbacec88b7111455

[TPolzer]

I am talking about the case where TrackerAdapterWorker holds the lock and bails out because of an Exception. This leads to the lock not being released. To cite https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Lock.html:

With this increased flexibility comes additional responsibility. The absence of block-structured locking removes the automatic release of locks that occurs with synchronized methods and statements. In most cases, the following idiom should be used:

     Lock l = ...;
     l.lock();
     try {
         // access the resource protected by this lock
     } finally {
         l.unlock();
     }