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.

Sign up for GitHub

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

Jump to bottom

[DO NOT REVIEW] GEODE-7001: region size experiment #3846

Closed
wants to merge 10 commits into from
Closed

[DO NOT REVIEW] GEODE-7001: region size experiment #3846

wants to merge 10 commits into from

Conversation

demery-pivotal
Copy link
Contributor

@demery-pivotal demery-pivotal commented Jul 24, 2019 •
edited

Region entry gauge stat and meter get their values from a new getLocalSize() method added to InternalRegion.

We will want to run some performance tests on this before committing. The getLocalSize() method on LocalRegion iterates over the segments in the abstract region map. The getLocalSize() method in PartitionedRegion iterates over the local buckets, each of which iterates over its abstract region map. This computation may or may not be expensive. We want to find out.

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

@demery-pivotal demery-pivotal force-pushed the GEODE-7001-region-size-experiment branch from b534477 to bfb7a80 Compare July 25, 2019 00:04
@jake-at-work
Copy link
Contributor

@dhemery I think the place to focus is CustomEntryConcurrentHashMap::size. It will attempt to get the count without locking but under update contention will resort to locking all the segments. I have a branch with some JMH benchmarks for this class already. If you want I can share and you can add one for calling size on an interval and see how that impacts puts.

@jake-at-work
Copy link
Contributor

@dhemery I made a small modification to the JMH I already had for comparing performance of CustomEntryConcurrentHashMap. I created a background thread that called size at 0ms (disabled, 1ms and 1s intervals. The benchmark used 100 threads to put or get concurrently into the maps. The size call had no measurable effect on the the put or get performance of CustomEntryConcurrentHashMap, which tops out on my machine 30M puts/s and 30M gets/s. It also shows no measurable performance impact on ConcurrentHashMap. So baring any serious locking going on at the RegionBucket down to the CustomEntryConcurrentHashMap you all should be good to go.

metatype
metatype reviewed Jul 25, 2019
View reviewed changes
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
@@ -8257,6 +8258,11 @@ public int size() {
}
}

@Override
public int getLocalSize() {
return getRegionMap().size() - tombstoneCount.get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

kirklund and others added 8 commits July 26, 2019 09:01
@kirklund @demery-pivotal
Extract reusable rules from GatewayReceiverMetricsTest
35fe613
@moleske @kirklund
@mhansonp @demery-pivotal
Add region entry count gauge
75f9b38 
Add 'member.region.entries' gauge in RegionPerfStats
- Add region.name and data.policy tags
- Add an AtomicLong to track the entry count
- Configure the member.region.entries gauge to fetch from the AtomicLong
- Configure the 'entryCount' stat to be supplied by the AtomicLong

Also refactored the following:
- Reorganize CachePerfStats/RegionPerfStats constructors
- Remove CachePerfStats.getEntries()
- Remove use of CachePerfStats.getEntries() from
  PartitionedRegionStatus.
- Add @OverRide to ValidatingStatisticsType methods in StatisticsTypeImpl
- Extracted invokeSuppliers() to new SuppliableStatistics interface and
  rename as updateSuppliedValues()
- Move responsibility to prepend 'RegionStats-' onto region statistics
  textID into the classes that create RegionPerfStats and
  CachePerfStats.

Co-authored-by: Michael Oleske <moleske@pivotal.io>
Co-authored-by: Kirk Lund <klund@apache.org>
Co-Authored-By: Mark Hanson <mhanson@pivotal.io>
@moleske @demery-pivotal
GEODE-7001: Use LongAdder instead of AtomicLong
883c196 
Co-authored-by: Michael Oleske <moleske@pivotal.io>
Co-authored-by: Dale Emery <demery@pivotal.io>
@demery-pivotal @kirklund
Region entry count meter and stat fetch from region
bd8ff92 
Added getLocalSize() method to InternalRegion.

Configured the "region entry count" stat and meter to fetch their values
from that method.

Co-Authored-By: Kirk Lund <klund@apache.org>
@demery-pivotal @kirklund
Add region entry count gauge test for LOCAL region
4a75142 
Co-Authored-By: Kirk Lund <klund@apache.org>
Co-Authored-By: Aaron Lindsey <alindsey@pivotal.io>
@demery-pivotal @kirklund
Remove unnecessary gfsh parameters to start server
a731ee3 
Co-Authored-By: Kirk Lund <klund@apache.org>
Co-Authored-By: Aaron Lindsey <alindsey@pivotal.io>
@demery-pivotal @kirklund
Simplify member shutdown in region entry count tests
baf7fa1 
Co-Authored-By: Kirk Lund <klund@apache.org>
Co-Authored-By: Aaron Lindsey <alindsey@pivotal.io>
@demery-pivotal
Remove MetricsPublishingServiceJarRule
c158339 
Migrated all uses to the more general ServiceJarRule.

Also changed ServiceJarRule.createJarFor(…) to return a Path instead of
a String.
@demery-pivotal demery-pivotal force-pushed the GEODE-7001-region-size-experiment branch from 15c9e56 to c158339 Compare July 26, 2019 16:10
demery-pivotal and others added 2 commits July 26, 2019 10:00
@demery-pivotal @kirklund
Restore PartitionedRegionDataStore.getSizeOfLocalBuckets()
507b4b6 
Co-Authored-By: Kirk Lund <klund@apache.org>
Co-Authored-By: Aaron Lindsey <alindsey@pivotal.io>
@demery-pivotal @kirklund
Inline unnecessary function jar rule
c5e17bf 
Co-Authored-By: Kirk Lund <klund@apache.org>
Co-Authored-By: Aaron Lindsey <alindsey@pivotal.io>
@demery-pivotal
Copy link
Contributor Author

Re-submitted as #3850

@demery-pivotal demery-pivotal deleted the GEODE-7001-region-size-experiment branch August 1, 2019 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metatype metatype metatype left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@demery-pivotal @jake-at-work @metatype @kirklund @moleske