Bug 325661

Comment 1 Matthew Khouzam 2010-10-29 15:12:57 EDT

Created attachment 182076 [details]
Backend performance improvements

This patch improves the speed of TMF's backend, specifically LTTng by removing unnecessary clones from the indexing. It also optimizes the trace selection so that if there is only one trace it will be selected. It forces the histogram to redraw periodically and overloads some equals to save time with the "instanceOf" command, something that affects performance when we are running this billions of times.

Comment 2 Francois Chouinard 2010-10-29 17:11:37 EDT

Comment on attachment 182076 [details]
Backend performance improvements

Committed on Indigo

Comment 3 Bernd Hufmann 2010-11-23 15:20:07 EST

Created attachment 183706 [details]
Statistics improvement and better tuning of background request splitting

- Statistics Improvement:
In the Statistic View the statistics are displayed in a tree structure. Each node in the tree can be reached by a path. This path is implemented as an array of Strings. Each path has a constant part and an dynamic part. The dynamic part depends on the content of the LTTng events. 

In the new solution (Bug325661_more_improvements.patch), the strings are replaced by unique integer representation (basic int type). This speeds the statistic collection up, because integer comparisons are done instead of string comparison and because less Java objects are instantiated.

- Better tuning of background request splitting
In order to keep the GUI responsive, long background requests are split in sub-requests that N events. In the attached patch (Bug325661_more_improvements.patch) N was increased so that the overhead of handling sub-requests is decreased.

- Increased checkpoint distance, i.e. more events between checkpoints. This reduces the overall memory usage (while not compromising the usability).

Comment 4 Francois Chouinard 2010-11-23 17:09:03 EST

Comment on attachment 183706 [details]
Statistics improvement and better tuning of background request splitting

Patch committed. Thanks Bernd.

Comment 5 Matthew Khouzam 2010-11-25 11:23:33 EST

Created attachment 183866 [details]
Priority Queue replacement

This patch improves the performance of the "GetNext()" side of the backend. 
Currently we use a Java PriorityQueue<JniEvent> and that datastructure has a time O(N) to add an element. 
The proposed patch reduces the time to O(Log(N)). With multi-cpu traces, this speeds up the getNext() calls by 5-10% (translates to a <1% total speed improvement generally on a trace with less than 8 cores.

Comment 6 Matthew Khouzam 2010-11-25 11:27:54 EST

Created attachment 183867 [details]
Seek only when necessary

Currently, Lttng has segmented backend requests. The consequence being that it calls a seek at each new beginning of a segment. Seeks on classic hard drives are rater slow (within 10ms) and this performance hit can add up very quickly. This patch eliminates seeks that go to the current position on the hard drive. The actual performance improvement can be up to 50% (reduce total time from 20 seconds to 10 seconds).

Comment 7 Francois Chouinard 2010-11-25 16:17:36 EST

(In reply to comment #5)
> Created an attachment (id=183866) [details]
> Priority Queue replacement
> 
> This patch improves the performance of the "GetNext()" side of the backend. 
> Currently we use a Java PriorityQueue<JniEvent> and that datastructure has a
> time O(N) to add an element. 
> The proposed patch reduces the time to O(Log(N)). With multi-cpu traces, this
> speeds up the getNext() calls by 5-10% (translates to a <1% total speed
> improvement generally on a trace with less than 8 cores.

[1] I notice that 2 private methods are never used (search() and searchBack()). Any reason to keep them?

[2] I would like some JUnit tests for JniPriorityQueue. After all you are proposing to replace something that has been extensively tested and I assume that, for licensing reasons, you had to write this code from scratch.

Thanks.

Comment 8 Bernd Hufmann 2010-11-30 08:14:45 EST

Created attachment 184120 [details]
Improved hashCode() for FixedArray used for Statistics

This patch replaces the hashCode() implementation of the class FixedArray to use Arrays.hashCode() instead. The new hashCode() implementation generates less collisions than the original implementation and hence there are less calls of the equals() method of the FixedArray class. The overall performance gain from the new implementation is up-to 20% (depending on the trace).

Comment 9 Bernd Hufmann 2010-12-06 11:45:51 EST

Created attachment 184630 [details]
Improved hashCode() for FixedArray used for Statistics (update)

This patch contains the same improvement for the hashCode() method than the previously submitted patch (reference 184120), but doesn't contain an unnecessary change for the equals() method.

Comment 10 Bernd Hufmann 2010-12-07 09:17:45 EST

(In reply to comment #6)
> Created an attachment (id=183867) [details]
> Seek only when necessary
> 
> Currently, Lttng has segmented backend requests. The consequence being that it
> calls a seek at each new beginning of a segment. Seeks on classic hard drives
> are rater slow (within 10ms) and this performance hit can add up very quickly.
> This patch eliminates seeks that go to the current position on the hard drive.
> The actual performance improvement can be up to 50% (reduce total time from 20
> seconds to 10 seconds).

This is a good idea to improve the performance. 

I tried the patch, but the result is that overall less events are parsed than the actual number of events in the trace when the number of events are bigger than the checkpoint interval size (currently 50000). This is can be seen in the Statistics View of LTTng. When omitting a seek in class JniTrace it seems that it ends up at the wrong position in the trace file. I didn't investigated the root cause of this, because I found a solution on the framework level (TMF) which avoids unnecessary seeks, as it was intended by your patch. I'll attach this solution to this bug soon.

Thanks

Comment 11 Bernd Hufmann 2010-12-07 11:20:00 EST

Created attachment 184728 [details]
Seek only when necessary (TMF solution)

This patch avoids unnecessary seeks between sub-requests of long background requests. The solution is done on the TMF level so that all trace implementations (not only LTTng) will profit from that change. The TmfExperiment class has been updated for that.

Comment 12 Bernd Hufmann 2010-12-10 09:01:34 EST

Comment on attachment 184630 [details]
Improved hashCode() for FixedArray used for Statistics (update)

Patch committed

Comment 13 Bernd Hufmann 2010-12-13 10:17:30 EST

(In reply to comment #11)
> Created an attachment (id=184728) [details]
> Seek only when necessary (TMF solution)
> 
> This patch avoids unnecessary seeks between sub-requests of long background
> requests. The solution is done on the TMF level so that all trace
> implementations (not only LTTng) will profit from that change. The
> TmfExperiment class has been updated for that.

Francois, could you please review this patch before I commit it?
Thank you!

Comment 14 Francois Chouinard 2010-12-13 10:36:29 EST

Fine with me. Go ahead and commit. Thanks.

(In reply to comment #13)
> (In reply to comment #11)
> > Created an attachment (id=184728) [details] [details]
> > Seek only when necessary (TMF solution)
> > 
> > This patch avoids unnecessary seeks between sub-requests of long background
> > requests. The solution is done on the TMF level so that all trace
> > implementations (not only LTTng) will profit from that change. The
> > TmfExperiment class has been updated for that.
> 
> Francois, could you please review this patch before I commit it?
> Thank you!

Comment 15 Bernd Hufmann 2010-12-13 11:41:14 EST

Comment on attachment 184728 [details]
Seek only when necessary (TMF solution)

Patch committed on trunk

Comment 16 Matthew Khouzam 2010-12-14 14:33:56 EST

Created attachment 185159 [details]
A fix of the events heap with test cases.

Comment 17 Bernd Hufmann 2010-12-14 14:41:57 EST

Comment on attachment 184728 [details]
Seek only when necessary (TMF solution)

Updated iplog

Comment 18 Bernd Hufmann 2010-12-15 10:03:15 EST

(In reply to comment #16)
> Created an attachment (id=185159) [details]
> A fix of the events heap with test cases.

Hi Matthew

I reviewed your patch and I did some tests. Functionally, I didn't find any problems. 

There is one thing I'd like to ask you to change. Please integrate your new JUnit  class in AllLTTngTests.java and AllJniTests.java, so that it will be executed together with all other test cases as part of a build. For that, you might have to do some minor tweaks in your JUnit test class.

Thanks
Bernd

Comment 19 Bernd Hufmann 2010-12-15 14:41:16 EST

(In reply to comment #18)
> (In reply to comment #16)
> > Created an attachment (id=185159) [details] [details]
> > A fix of the events heap with test cases.
> 
> Hi Matthew
> 
> I reviewed your patch and I did some tests. Functionally, I didn't find any
> problems. 
> 
> There is one thing I'd like to ask you to change. Please integrate your new
> JUnit  class in AllLTTngTests.java and AllJniTests.java, so that it will be
> executed together with all other test cases as part of a build. For that, you
> might have to do some minor tweaks in your JUnit test class.
> 
> Thanks
> Bernd

Hi Matthew

In method public boolean seekToTime(JniTime seekTime){}, please remove also your changes that by-passes the seek if the current time is equal to the time to seek. This is not needed and covered by patch attachment 184728 [details]. See also comment #10 and #11 for more information about this.

Here is the code that needs to be removed:
        if(currentEvent!= null )
        {
        	if( currentEvent.getEventTime().equals(seekTime))
        		return false;
        }

Thanks
Bernd

Comment 20 Matthew Khouzam 2010-12-20 11:49:34 EST

Created attachment 185563 [details]
A fix of the events heap with test cases. 

Previous test case was invalid, all events were the same. This time each event is from a different tracefile and thus a completely different event.

Comment 21 Matthew Khouzam 2010-12-20 11:51:38 EST

Created attachment 185564 [details]
A fix of the events heap with test cases.

This fix contains more robust test cases that do not contain duplicates.

Comment 22 Patrick Tasse 2013-05-17 13:22:58 EDT

Performance improvements were implemented for the release of Legacy LTTng which is being removed in Linux Tools 2.0.

If more performance improvements are required it should be tracked in a new bug.

Included in 0.7 release.