Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

6674: Optimize finding of first start time and first & last end times #31

Closed
wants to merge 8 commits into from

Conversation

@Gunde
Copy link

Gunde commented Jan 14, 2020

This optimization relies on the fact that event lanes are sorted by timestamp. This allows us to skip performing very costly comparisons when finding the first start time or first/last end times of an IItemCollection and instead just get the first or last events in each lane and find the event in that set that was earliest or last, respectively.

This PR also removes the associated aggregators to strongly encourage downstream users to make use of the added RulesToolkit methods.

Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

JMC-6674: Inefficient finding of first start time and first & last end times

Approvers

  • Marcus Hirt (hirt - Reviewer)
Gunde added 2 commits Jan 14, 2020
This patch optimizes the finding of the first start time, first end time, and last end time in an IItemCollection based on the assumption that it stores events disjointly, sorted by timestamps.
Fix formatting
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 14, 2020

👋 Welcome back hdafgard! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Jan 14, 2020
@Gunde Gunde requested a review from thegreystone Jan 14, 2020
@mlbridge
Copy link

mlbridge bot commented Jan 14, 2020

Webrevs

Update javadocs
@thegreystone
Copy link
Collaborator

thegreystone commented Jan 14, 2020

Careful here. They are not sorted on start and end time both, and events are not guaranteed (except that we recommend that this is the case for the events of a particular event type, if at all possible) to be non-overlapping.

@Gunde
Copy link
Author

Gunde commented Jan 15, 2020

JMC parser currently guarantees that IItemCollection events are retrieved in sorted, disjoint, lanes. Each event is iterated over in the time that it was emitted, and if there is an overlap then it will be in another lane. This is why we iterate through the IItemCollection and then through the IItemIterable, even if the IItemCollection is filtered to only contain one type.

There is still the problem that this relies on the implicit assumption that these lanes are sorted in this fashion, but since we're currently performing a computationally intensive sorting during the parsing we should think about exploiting that fact.

@thegreystone
Copy link
Collaborator

thegreystone commented Jan 20, 2020

Can we add some verification tests showing that we get the same results for a few sets of data? I think it would be nice if we added a recording with (custom, well defined) overlapping events (of all types of overlap) to the test recordings.

@openjdk
Copy link

openjdk bot commented Jan 27, 2020

@Gunde This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

6674: Optimize finding of first start time and first & last end times

Reviewed-by: hirt
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 9 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to do this manually, please merge master into your branch first.

➡️ To integrate this PR with the above commit message, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jan 27, 2020
@Gunde
Copy link
Author

Gunde commented Jan 27, 2020

/integrate

@openjdk openjdk bot closed this Jan 27, 2020
@openjdk openjdk bot added integrated and removed ready labels Jan 27, 2020
@openjdk
Copy link

openjdk bot commented Jan 27, 2020

@Gunde The following commits have been pushed to master since your change was applied:

  • 875abcd: 6670: Harmonize ~ Unclassifiable ~ across Flame View and Stacktrace View
  • 2dd271f: 6572: Make mbean functions protected by permission checks
  • 1676ecc: 6673: Upgrading to Tycho 1.6.0
  • e647a9d: 6661: Simplify Agent JMX tests
  • c2134e2: 6672: JavaScript formatting template
  • afcf74d: 6660: Make JMX API not return classes
  • ea933fa: 6674: Use try-with-resource in more places and close un-closed resources
  • 8790592: 6668: Move org.openjdk.jmc.jdp bundle from application to core
  • f74caa8: 6549: Color flame chart based on package name

Your commit was automatically rebased without conflicts.

Pushed as commit 6dd19dd.

@mlbridge
Copy link

mlbridge bot commented Jan 27, 2020

Mailing list message from Henrik Dafgård on jmc-dev:

Changeset: 6dd19dd
Author: Henrik Dafg?rd <hdafgard at openjdk.org>
Date: 2020-01-27 21:24:09 +0000
URL: https://git.openjdk.java.net/jmc/commit/6dd19dd5

6674: Optimize finding of first start time and first & last end times

Reviewed-by: hirt

! application/org.openjdk.jmc.flightrecorder.ext.g1/src/main/java/org/openjdk/jmc/flightrecorder/ext/g1/G1Page.java
! application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/selection/FlavorToolkit.java
! core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/dataproviders/HaltsProvider.java
! core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/general/ClassLoadingRule.java
! core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/latency/JavaBlockingRule.java
! core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/memory/GcFreedRatioRule.java
! core/org.openjdk.jmc.flightrecorder.rules.jdk/src/main/java/org/openjdk/jmc/flightrecorder/rules/jdk/memory/IncreasingLiveSetRule.java
! core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java
! core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/SlidingWindowToolkit.java
! core/org.openjdk.jmc.flightrecorder/src/main/java/org/openjdk/jmc/flightrecorder/jdk/JdkAggregators.java
! core/tests/org.openjdk.jmc.flightrecorder.test/META-INF/MANIFEST.MF
! core/tests/org.openjdk.jmc.flightrecorder.test/pom.xml
+ core/tests/org.openjdk.jmc.flightrecorder.test/src/test/java/org/openjdk/jmc/flightrecorder/test/OverlappingEventsTest.java
+ core/tests/org.openjdk.jmc.flightrecorder.test/src/test/resources/recordings/overlap.jfr

@Gunde Gunde deleted the Gunde:improve-time-aggregations branch Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.