Editing User:Pulkit Mittal/GSOC2014/logs

=Development Logs=
==Project Details==
{| class="wikitable"
|-
|'''Project Name''' || STEP Libraries: Improving Thread Safety and Performance 
|-
|'''Project Student'''|| Pulkit Mittal
|-
|'''IRC(nick)'''|| hoiji
|-
|'''Github (handle)'''|| hoiji09

|}

==Milestones==
===Compulsary===
*Thread Safety in cllazyfile ''Status'': Review Awaiting 
*Thread Safety in clstepcore ''Status'': Started
*Thread Safety in cleditor ''Status'': Not Started
*Thread Safety in cldai ''Status'': Not Started
*Introduce multi-threading cllazyfile ''Status'': Not Started

===Bonus===
In this section I will list down all the single threaded performance optimizations which are done in the GSOC period.



==GSOC Period==
===Week 1===
'''17 May (Sat):''' 
*Figured out various functionalities in cllazyfile step library which should be made thread safe. <br> These are
*#Opening multiple files in parallel.
*#(lazy) loading separate SDAI_Application_instance in parallel.
*#Reading the forward / backward references of instances in parallel.
*#Finding instances belonging to same / different types in parallel.
*#Initializing / Setting the registry in parallel.
*#Registering Data Sections in parallel

*Strategizing how the various features will be added into the code. Firstly a test will be created to note the existance of thread safety for the above features. If the test fail then appropriate coding will be done

'''18 May (Sun):'''
*A test was created in ''lazy_thread_safety_test.cc'' file, to check the concurrent read safety of ''getFwdRefs()'' and ''getRevRefs()'' and expectedly it failed.
*''Note:'' to run the test, the CMakeFiles: ''cmake/SC_CXX_schema_macros.cmake'' & ''src/cllazyfile/CMakeLists.txt'' were slightly messed up.

'''19 May (Mon):''' 
*Introduced ''getFwdRefsSafely'' and ''getRevRefsSafely'' which are thread safe. The thread safety test in ''lazy_thread_safety_test.cc'' passed. Submitted code through git in a branch called hj/cllazyfile-thread-safety. 
*Discussed with mentor about the importance order of functionalities mentioned above. Hence will be covering the functionalities in the following order 3(done)>4>2>1>6

'''20-21 May (Tue, Wed):'''
*Noticed that ''getFwdRefsSafely'' and ''getRevRefsSafely'' are consume memory each time they are called (as separate clones are made for each invocation). (Blame the use of Judy Structure) The memory is freed only when destructor of ''lazyInstMgr'' is called. One way around this problem was to let the user call a function (say) ''returnFwdRefsSafely'' which would release the space related to a clone. Hence, I spent 2 days trying to modify the original judy code so that it allows closure of clones. But as I was not able to get a breakthrough, I have left this task for later. 

'''22 May (Thu):''' 
*Created a test to check the thread safety of ''getInstances'' (''i.e.'' finding instances belonging a specific type). As expected the test failed when the number of invocations were very high. This is because ''getInstances'' internally uses ''find'' operation which is not thread safe. 

'''23 May (Fri):''' 
*Added the thread-safe ''getInstancesSafely'' and ''countInstancesSafely''. Also submitted the code in github.


===Week 2===
'''24 May (Sat):'''
* Started working towards making lazy-loading of instances thread safe. 
** Created a test which would spawn two threads which try to load instances. 
*** In the first half of iterations the threads would try to load same set of instances.
*** In the second half of iterations the threads would try to load different set of instances (not necessary disjoint).
*** For each thread within one iteration an instance would be loaded twice. This checks the case where the instance may or may-not be already loaded and the case where the instance is already loaded. 
** The problem that arose during the compilation of the test was that, finally the ''_mainRegistry'' was being used, and I had very little idea about the schemas and registries. It could not find the ''schema.h'' file.
* Couldn't work the earlier half of the day as I was traveling.

'''25 May (Sun):''' 
* Got the thread safety test for lazy-loading to compile by doing certain changes in the ''src/cllazyfile/CMakeLists.txt''.
** This was done by including the directory ''${CMAKE_BINARY_DIR}/schemas/sdai_cd209'' for the above test.
** The restriction that the above changes imposed on ''lazy_thread_safety_test'' was that, now it has to be given the the step file ''data/cd209/ATS1-out.stp'' as input.
* A potential bug was found while running the above test. The bug would appear if we follow the following steps
*# ''lazyInstMgr * mgr = new lazyInstMgr;''
*# ''mgr->initRegistry( SchemaInit );''
*# ''mgr->openFile( fileName );''
*# ''delete mgr''
*# ''lazyInstMgr * mgr = new lazyInstMgr;''
*# ''mgr->initRegistry( SchemaInit );''
** The last step would cause assertion failure in function ''setRegistery''. (i.e. ''assert( _mainRegistry == 0 )''). This was solved by explicitly initializing ''_mainRegistry'' with '0' inside the ''lazyInstMgr'' constructor.
* When the test was run on the original ''loadInstance'' function, it gave assertion error related to the input stream.
* Saw the comments posted by Mark on my earlier commits. I decided to make corresponding changes in the code, once I had made lazy-loading thread safe

'''26 May (Mon):'''
* A thread-safe version of ''loadInstance'' was created. For this a fat lock was used. It is possible that we can make the lock application finer, however that has been left for later. This version passed the test successfully.
** However their appeared to be some problem in the step file, because loading of one particular instance always failed, irrespective of the number of threads. A temporary fix was applied by removing a newline character from the step file. 
** In the ''loadInstance'' function, their was an issue of a warning ''instance #... not found in any section'' appearing even if the instance was found in the ''_instancesLoaded'' list. The fix was trivial
* A thread safe counterpart to ''mgr->getAdapter()->FindFileId( instanceID )->GetSTEPentity()'' was made. This was slightly complicated then the previous features as it consisted of 2 complex steps in different classes. Both being thread unsafe.
** For this each thread was assigned its own mgrNodeHelper. This mgrNodeHelper was created once, for each thread and reused on later invocations. 
** Dependency was taken on thread safe ''loadInstanceSafely''
** The counterpart was also successfully tested

'''27 May (Tue):''' 
* As per the advice given by Mark, the macro ''HAVE_STD_THREAD'' was utilized in ''CMakeLists.txt'', including header files, mutex / thread safe functions declarations etc. 
** This opportunity was also used to clean up the ''CMakeLists.txt'' 
* Fixed the newline issue encountered on 26th May. This was done by modifying the lazyfile parser by instructing it to ignore the newlines in such cases. The temporary fix was reverted back as it was no longer required.
* Prepared the changes done from 24th May in the form of multiple commits. Also the thread safe code was clubbed together wherever possible for easy identification.

'''28 May (Wed):'''
* While introducing ''HAVE_STD_THREAD'' in ''src/cllazyfile/lazyTypes.h'' a compilation problem was encountered. Somehow ''HAVE_STD_THREAD'' macro was not being recognized by ''lazyTypes.h''. This was surprising as ''HAVE_STD_THREAD'' was recognized by ''lazyInstMgr.cc''. The final solution was to explicitly declare ''HAVE_STD_THREAD'' through the compiler flag in ''src/cllazyfile/CMakeLists.txt''. 
* Also continuing from the last point in 27th May, the work done from 24th May was organized into 12 different commits and submitted to github.

'''29 May (Thu):'''
* Started adding a test for checking thread safety in while opening multiple files in parallel, using the same lazyInstMgr. 
** The first and foremost problem was identifying the data structures of the lazyInstMgr whose state would change when ''lazyInstMgr::openFile()'' is called. Unlike the previous functionalities here more then one data structures were involved. The following data structures were identified.
*** ''instanceRefs_t  _fwdInstanceRefs''
*** ''instanceRefs_t _revInstanceRefs''
*** ''instanceTypes_t * _instanceTypes''
*** ''instanceStreamPos_t _instanceStreamPos''
*** ''dataSectionReaderVec_t _dataSections''
*** ''lazyFileReaderVec_t _files''
*** ''unsigned long _lazyInstanceCount''
*** ''int _longestTypeNameLen''
*** ''std::string _longestTypeName''
** The test would create two instances of lazyInstMgr, through first it would call openFile for two different files in a serial manner, through second it would call openFile for these two files parallely. Later it would compare the above data structures of both the instance managers and report any discrepancy it finds. 

'''30 May (Fri):'''
* The above test was completed. 
** An extra lazyInstMgr API was added to get ''_instanceStreamPos'' data structure for comparison.
** The test when run on existing openFile failed. Multiple reasons were attributed to the failure by running the test multiple times. Apart from gracefully failing, the test also failed many times due to ''segmentation fault'' or an assertion failure in the ''cllazyfile parser''.

* Work was started on making the openFile thread safe. 


===Week 3===
'''31 May (Sat):'''
* At this I was going through the dilemma of following the convention of creating different functions for thread safe code vs. using macros for demarcating code used for ensuring thread safety.
** Continuing the former meant code duplication.
** Switching to the latter meant that flexibility of letting the user decide which function he wants to call is thrown away. 
** I finally took the middle way decision which didn't had any of the above problem. 
*** All calls to the ''mutex'' are now through a wrapper which directs the call to the ''mutex'' functionality if ''HAVE_STD_THREAD'' is defined or else to a dummy function which does nothing. 
* In the thread safe counterpart of ''openFile'', ''openFileSafely'' fine grain locking was done.
** Mutexes were added at to safe-gaurd their respective data structures
** Functionalities such as ''lazyInstMgr::registerDataSection'' were converted into a two step process.
** Since it was now possible that an element of the ''_dataSection'' vector can be null the destructor for ''lazyInstMgr'' was changed accordingly.

'''1 June (Sun):'''
* The test for checking thread safety was still failing, due to assertion failure in the ''cllazyfile parser''.
** It was found that this is due to a ''static'' variable in ''sectionReader::getDelimitedKeyword''. The function was reworked and named ''fillDelimitedKeyword''. The functions taking dependence on this were slightly modified to reflect this change.
** Another static variable in ''p21HeaderSectionReader'' was identified and tackled with.
* The test finally succeeded. Some performance statistics
** File A: ''3tnv70-asa.stp'' (~45mb) was taken from the site examples.
** File B: ''gtc01-mme01_asm.stp'' (~45mb) was taken from ''http://downloads.openmoko.org/developer/CAD/Neo1973_IGES_STEP.zip'' 
** It took ~9 seconds if we tried to open either File A or File B.
** It took ~17 seconds if we tried to open File A and then File B.
** It took ~12.5 seconds when both the files were opened in parallel.

'''2 June (Mon):'''
* The locking in ''loadInstanceSafely( instanceID )'' was made finer. 
** Double checking was also done to enhance performance in cases in which say:
**# Thread A is creating a ''realObject'' belonging to instance ''x''. 
**# Thread B comes and wants to get an object belonging to instance ''y''. 
*** If instance ''y'' has been already loaded then Thread B doesn't have to wait for Thread A. This is now handled. 
* The work done from 29th May was organized into different commits and pushed to github.

'''3 June (Tue)'''
* Going through the code in ''src/clstepcode''
** Learnt about the various files in the library.
** Several Problems were identified:
*** The amount of code is huge.
*** What makes the problem worse is that there are lots of small classes. Since a TDD approach is being followed it means that there will have to be lots of test files. 

'''4-6 June (Thu - Fri)'''
* Very Little work done due to family reunion + travelling.


===Week 4===
'''7 June (Sat)'''
* Unsuccesfully tried to figure out a way to run the tests such as ''src/clstepcore/test/test_operators_STEPattribute.cc'' on my remote machine.
* Created a mutex wrapper: ''sc_mutex'' that will contain ''std::mutex'' and call its lock, unlock function if ''HAVE_STD_THREAD'' is enabled. If on the other hand ''HAVE_STD_THREAD'' is disabled, the wrapper will not contain ''std:mutefx'' and will call the dummy lock, unlock functions.   
** I will modify cllazyfile to take dependency on it later. 

'''8 June (Sun)'''
* Changed the branch name from ''hj/cllazyfile-thread-safety'' to ''hj/thread-safety''. Now I will keep all the thread-safe code in one branch. 
* Modified the ''CMakeLists.txt'' of each of the library. 
** It was also found that some of the tests used the header files directly. Hence the cmake files of such libraries will have to be changed when required.

'''9 June (Mon)'''
* The test for checking thread safety of ''instmgr'' was started.
** The functions whose thread safety would be checked was append and delete functions.
** Three test scenrios were checked:
**# Two threads appending ''SDAI_Application_instance'' pointers to the same ''instmgr''.
**# Two threads deleting ''SDAI_Application_instance'' pointers to the same ''instmgr''.
**# One thread appeding ''SDAI_Application_instance''pointers and other thread trying to delete some other pointers from the same ''instmgr''.

'''10 June (Tue)'''
* The test for checking thread safety of ''instmgr'' was completed. It was named as ''InstMgr_thread_safety_test'' and put into ''src/clstepcore'' directory.
*# In each of the test cases before the insertion / deletion happens on one InstMgr sequentially and other parallely. 
*# These two are later compared for any disparity. 
* On each run either ''InstMgr_thread_safety_test'' failed gracefully (i.e a disparity was found between two InstMgr) or a seg. fault was encountered.

'''11 June (Wed)'''
* Their was some duplicate code in ''instmgr'' class for ''GetSTEPentity'' function. Even though the function is obselete, the duplicated code was removed. 
* Code was written to make ''InstMgr'' Append and Delete function thread safe.
** This involved some amount of code refractoring and introduction of a mutex.
** A compilation error on invoking ''make'' prevented any further progress.

'''12 June (Thu)'''
* Was able to finally compile yesterdays code by changing the file ''SC_CXX_schema_macros.cmake'' in the cmake directory.
* The ''InstMgr_thread_safety_test'' passed in all three cases. Some incorrect code was also corrected.

'''13 June (Fri)'''
* I decided that I will work in the ''GenNode'' related classes (''GenNodeArray'', ''GenNodeList'') defined in clutils and the classes which inherit from them:
** ''MgrNode'': ''MgrNodeArray'', ''MgrNodeArraySorted'', ''MgrNodeList''
** ''DispNode'': ''DispNodeList''
* Some of the code was very badly written (seeing from a thread safety perspective) 
** Ex. The Remove function in ''GenNode'' modified the state of other ''GenNode'' objects. Typically this should be done by the ''GenNodeList'' class. 


=== Week 5 ===
'''14 June (Sat)'''
* Added ''containingList'' pointer in ''GenericNode''. This pointer will point to the list of which the node is part of. 
** This pointer will used whenever the ''GenericNode'' or its descendent want to perform any operation on the other Nodes.
** Such operations will now be directed to the list function whose responsibility would be to ensure proper locking.
** Added assertions and checks regarding the same.
** Also used ''containingList'' to remove duplicate code.
* Some of the code was moved between files, to make the code neater.

'''15 June (Sun)'''
* Organized the work done until 12 June into commits and pushed it into the github repository. 
** This was done so that ''Mark'' could review one of the changes
* Added sc_recursive_mutex.
** This will be used to handle situtations where there are 2 public APIs (say A, B). 
*** Both modify an internal data structure, hence lock it. 
*** However one API depends on other. (say B depends on A)
** Earlier In such cases I used to make a helper function (H) which would do the main job. A and B would depend on H and can be individually locked.
** Having a recursive lock would eleminate the need to do the above.
*** Made ''instmgr'' lock recursive.

'''16 June (Mon)'''
* Corrected locking in ''getApplication_Instance''. Earlier (due to my carrelessness) their was a possibilty that a thread might never release the lock.
* Modified the insert function in gen/mgr/disp lists. 
** The new insert function checks whether the existing node belongs to the same list or not. If it doesn't it does nothing and returns false. 
** This will be very helpful when introducing thread safety in list structures. Ex. take the scenerio: 
*** Thread A wants to insert x after y.
*** Meanwhile Thread B removes y from current list.
*** In such a case the thread A's insertion fails.
* Finally introduced ''recursive_mutex'' in ''gennodelist''. ''mgrnodelist'' which inherits ''gennodelist'' was also changed accordingly.

'''17 June (Tue)'''
* Added ''GetGenNode'' function in ''GenNodeArray''. 
** This function is a safer couterpart to [] function. 
** Replaced call to the [] function with calls to ''GetGenNode'' function. 
** Was feasable since the former is only used for getting ''GenericNode'' pointers.
* Minor refractoring in ''MgrNodeArray'' This will be helpful when we introduce a mutex in ''GenNodeArray''.

'''18 June (Wed)'''
* Added ''std_recursive_mutex'' in ''GenNodeArray''. Also modified classes which inherit the above accordingly
* Organized the work done from 14 June into commits and pushed it into the github repository.

'''19 June (Thu)'''
* Could not work, as I devoted my the whole day to my M.Tech Project.

'''20 June (Fri)'''
* Next I decided to work on ''ExpDict.h/.cc/.inline.cc'' files. These define classes representing descriptors for EXPRESS schemas, entities, attributes, and types. 
** These file have around ~50 classes. On going through each of them it was found that the potentially thread unsafe classes can be divided into  categories.
**# Classes which define a set data structure for a subclass of ''Dictionary_instance'' 
**# Classes which inherit from ''SingleLinkList'' class.
**# Some subclasses of ''Dictionary_instance'' which take dependency on std::vector structures or multiple structres which should be consistent with each other.
 

== Week 6 ==
'''21 June (Sat)'''
* Added class ''Dictionary_instance__set''. This class is intended to be a superclass of all the *__set classes. 
** These classes are:
*** ''Explicit_item_id__set''
*** ''Implicit_item_id__set''
*** ''Interface_spec__set''
*** ''Global_rule__set''
*** ''Uniqueness_rule__set''
** Lot of duplicated code was removed this way. 
** Making ''Dictionary_instance__set'' thread safe would now ensure thread safety of the above subclasses.

'''22 June (Sun)'''
* Made ''Where_rule__list'' a subclass of ''Dictionary_instance__set'', as it was found to have similar features.
* Work done from 19 June was organized into commits and pushed to github.

'''23 June (Mon)'''
* Some missing code in yesterday commits was added and submitted to github.
* Made ''Dictionary_instance__set'' thread safe by adding a mutex.
* Made ''SingleLinkList'' class thread safe by adding a mutex. 
** Also a cmake file was modified inorder to compile the above changes.
* Made Schema thread safe by using 3 mutexes to protect the ''_function_list'', ''_procedure_list'' & ''_global_rules'' data structures. 
** The first two are vectors. 
** The last is already a thread safe structure, however its being lazy-initilized hence a mutex is needed.
* Made ''Global_rule'' thread safe by using a mutex. 
** Thread safety in functions ''entities_'' and ''where_rules_'' was being violated

'''24 June (Tue)'''
* The next focus was the ''Registry'' class. A test to check the thread safety of Registry class was created.
** In it the Entity, Schema and Type operations are tested.
** For each, there are 3 checks: Add-Add, Remove-Remove, Add-Remove. ** For every check the two operation are done first serially and then parallely. Their results are then compared. 
* The test when run either gracefully failed or gave a segFault in every check. 

'''25 June (Wed)'''
* ''The curious case of hashtables'': The ''Registry'' class took dependency on a hashtable data structure defined in clutils/sc_hash.h. 
** These hashtable methods appeared to pass the thread safety check, when the number of elements which were to be inserted in the hash table were small.
** However when lots of element were added into a hash table, an internal function of the hashtable ''SC_HASHexpand_table'' is called which is thread unsafe. 
** Hence it was decided to put a fat-lock around the hash-table.
*  Made Registry ''Entity'', ''Schema'' and ''Type'' operations thread safe by adding a mutex for each. 
** All the checks in ''Registry_thread_safety_test'' now pass.
* The code written from 23 June was organized into commits and pushed to github.

'''26 June (Fri)'''
* Wrote a blog on my mid-term progress: http://brlcad.org/wiki/User:Pulkit_Mittal/GSOC2014/Midterm