Bugzilla – Full Text Bug Listing
| Summary: | [ThreadProf] implementing BCI support at agent | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Alexander N. Alexeev <analexee> | ||||||||
| Component: | TPTP | Assignee: | Alexander N. Alexeev <analexee> | ||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P1 | CC: | analexee, asaf.yaffe, chris.l.elford, guru.nagarajan, jkubasta, paulslau, sluiman | ||||||||
| Version: | unspecified | Keywords: | plan | ||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| URL: | http://www.eclipse.org/tptp/groups/Architecture/documents/features/hf_200320.html | ||||||||||
| Whiteboard: | closed460 | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 200320 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Alexander N. Alexeev
Guru, could you assign this defect to me. Note, if this enhancement is a work item of 200320, it should be marked as a defect. Otherwise (e.g. new function not covered by 200320), you will have to request AG and PMC/PG approval to include them in 4.5. I believe this to be a component of 200320. It is marked as a defect already yes? (In reply to comment #3) > I believe this to be a component of 200320. It is marked as a defect already > yes? > Correct, this was originally opened as an enhancement. Can we hold off on becoming more dependent on BCI until we resolve how to handle the limitations presented to us by the vm vendors like IBM who have restrictions on what and when we can instrument. I am guessing this feature involves the core classes that are limitations in Java 1.5. Perhaps the solution will be to condition the client to not expose certain capabilities with certain runtimes but this should be figured out first. We need solutions that are confirmed to work with BEA/IBM/SUN and hopefully Apache Harmony. As I understand it, BCI is only required for the "critical path" enhancements for threading profiling. A question to Alex is whether we plan to have a checkbox in the thread profile workbench configuration dialog to enable critical path analysis. If so, it should be possible to grey out that checkbox for IBM that does not support instrumenting the Thread class as needed. In this case, IBM VM would still have thread views just not the critical path piece. If IBM Java6 allows JLT to be instrumented it would only be Java5 IBM that would have this option unavailable. Currently we plan to use BCI in ThreadProf only for contention analysis. All instrumentation should have option to be switched off both from WB and command line. The only thing is required to add in this context is how to define vendor of JRE and its capabilities from WB during profiling configuration. BTW current design doesn't assume system classes instrumentation. Created attachment 89931 [details]
runtime update for thread enh
Please, review the patch
1. Added one complex CA event in Martini with filtering
2. Added event Group for CA events
3. Added XML printing for events
4. Added TimedOut field handling for MonitorWaited event
(In reply to comment #8) My comments to the Martini-related changes: General Comments ================ 1) You changes does not follow the coding conventions used throughout the Martini code. Some examples: - enumerations use C++ style definition, starts with E and use MixedCase. - curly braces always appear on their own line ( no if () {...} ) - pointer attributes (*) are adjacent to the variable name (e.g, "int *pInt" instead of "int * pInt" or "int* pInt") - etc... 2) Please remove extra empty lines from the code (e.g., MpiAPI.h:2045-2048) 3) Why does the patch modify JIE.mak and JIEUtils.cpp? 4) Why does the patch include generated javah file for ThreadProxy? (not used in source) 5) Why does the patch include the build.cmd file for ThreadProxy? Need a pre-compiled Java 1.5 binary in CVS (as in CGAdaptor and HeapAdaptor). No need for a build file. API Changes (in MpiBase.h and MpiAPI.h) ======================================= 1) The names you selected for the new event, associated groups, filters and data items are inconsistent. I suggest using the following names/terms instead: Event Constant (MpiBase.h): EV_THREAD_INTERACTION Filter type (MpiBase.h): FT_THREAD_INTERACTION Event group (MpiAPI.h): EG_THREAD_INTERACTION Event observer (MpiAPI.h): IThreadInteractionEventObserver Event observer data (MpiAPI.h): (also see comment below): struct SThreadInteractionEventData { TId threadId; TId objectId; EThreadInteractionType interactionType; BitSet validData; }; You also need to add a DR_THREAD_INTERACTION_TYPE constant to MpiAPI.h. New "thread" filter: IThreadInteractionFilter Filter data: SThreadInteractionFilterData 2) The CA_METHOD_ID enum: - Definition is inconsistent with the API. Why are you using a C-style definition? Please change to: enum EThreadInteractionType { IT_NOTIFY_ALL = 1, IT_NOTIFY = 2, IT_INTERRUPT = 3, IT_START = 4 }; Dispatching new event (EventManager.cpp, EventDispatcher.cpp) ============================================================= 1) Remove the check for DR_MONITOR_TIMED_OUT item and always provide it (this data is available anyway, so you just add overhead by checking this) 2) Why do you need a dual Notify implementation? Everything should be implemented in the generic "Notify" method (e.g., as in "Object Alloc" event). Data gathering should happen in the Dispatcher::Notify. No need for a specialized NotifyCallCAMethod.... method. Thread Adaptor (ThreadAdaptor.cpp, ThreadProxy.h) ================================================= 1) ThreadProxy.h contains many unused constants. Keep only the ones you use and delete all others. 2) Validity check of CP callback entriesshould be done once in CThreadAdaptor::DoThreadInstrumentation. Then you can skip this check in all other places. 3) CThreadAdaptor::InstrumentMethodCalls: Use InstrumentationAdaptorBase functions to obtain method info and deallocate method info. No need to duplicate this logic. 4) CThreadAdaptor::ModifyByteCodes: to properly support RedefineClasses you need to return MRTE_ERROR_INSTRUMENTATION_NOT_NEEDED if none of the methods were instrumented (due to filtering). Otherwise, the system will try to hot-swap classes even when they were not actually instrumented. See CGAdaptor::DoCallGraphInstrumentation for an example. Thanks, Asaf Created attachment 90766 [details]
patch
Improved according Asaf's comments
Patch approved for check-in with the following comments (please fix before check-in): EventDispatcher.h ================= 1) Remove the redundant NotifyThreadInteractionObservers method definition from CThreadInteractionEventDispatcher ThreadAdaptor.cpp ================= 1) AddInstrumentationToMethod(): There are two error conditions that are not handled (marked with TODO). These errors should be handled by skipping the instruction and logging this as a "level 0 informative" error in the Martini log file. We also need to review/rewrite the Doxygen comments in MpiAPI.h (for the new APIs) and regenerate the SDK documentation. Please open a Bugzilla for this and assign to me. Thanks, Asaf required changes introduced patch committed in HEAD resolving as fixed Created attachment 91742 [details]
Documentation Update (patch)
Updated documentation for the new APIs introduced in this bugfix.
Alex, please review the Documentation Update patch and check-in these fixes. The patch contains only changes in comments. There are no code changes. (In reply to comment #15) > Alex, please review the Documentation Update patch and check-in these fixes. > The patch contains only changes in comments. There are no code changes. > Update for documentation is reviewed and committed in CVS HEAD. As of TPTP 4.6.0, TPTP is in maintenance mode and focusing on improving quality by resolving relevant enhancements/defects and increasing test coverage through test creation, automation, Build Verification Tests (BVTs), and expanded run-time execution. As part of the TPTP Bugzilla housecleaning process (see http://wiki.eclipse.org/Bugzilla_Housecleaning_Processes), this enhancement/defect is verified/closed by the Project Lead since this enhancement/defect has been resolved and unverified for more than 1 year and considered to be fixed. If this enhancement/defect is still unresolved and reproducible in the latest TPTP release (http://www.eclipse.org/tptp/home/downloads/), please re-open. As of TPTP 4.6.0, TPTP is in maintenance mode and focusing on improving quality by resolving relevant enhancements/defects and increasing test coverage through test creation, automation, Build Verification Tests (BVTs), and expanded run-time execution. As part of the TPTP Bugzilla housecleaning process (see http://wiki.eclipse.org/Bugzilla_Housecleaning_Processes), this enhancement/defect is verified/closed by the Project Lead since this enhancement/defect has been resolved and unverified for more than 1 year and considered to be fixed. If this enhancement/defect is still unresolved and reproducible in the latest TPTP release (http://www.eclipse.org/tptp/home/downloads/), please re-open. |