Skip to content

IGNITE-7986: GridPartitionStateMap.entrySet() optimization. #3659

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

Closed
wants to merge 7 commits into from
Closed

IGNITE-7986: GridPartitionStateMap.entrySet() optimization. #3659

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8a905fb
IGNITE-7986: GridPartitionStateMap.entrySet() optimization.
BiryukovVA Apr 26, 2018
76e9540
IGNITE-7986: review fix.
BiryukovVA May 3, 2018
914962f
IGNITE-7986: Removed copy of the field.
BiryukovVA May 3, 2018
31f5a43
IGNITE-7986: Review fix. Javadocs and test.
BiryukovVA May 3, 2018
51619ee
IGNITE-7986: Empty line removed.
BiryukovVA May 3, 2018
7bcac5a
IGNITE-7986: fix javadoc.
BiryukovVA May 3, 2018
226444c
IGNITE-7986: Review. Added test.
BiryukovVA May 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions modules/core/src/main/java/org/apache/ignite/internal/util/GridPartitionStateMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,35 +52,43 @@ public class GridPartitionStateMap extends AbstractMap<Integer, GridDhtPartition
@Override public Set<Entry<Integer, GridDhtPartitionState>> entrySet() {
return new AbstractSet<Entry<Integer, GridDhtPartitionState>>() {
@Override public Iterator<Entry<Integer, GridDhtPartitionState>> iterator() {
final int size = states.isEmpty() ? 0 : (states.length() - 1) / BITS + 1;

return new Iterator<Entry<Integer, GridDhtPartitionState>>() {
private int next;
private int idx;
Copy link
Contributor

@dspavlov dspavlov May 3, 2018

Choose a reason for hiding this comment

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

please add javadoc for this and other fields

private int cur;

@Override public boolean hasNext() {
Copy link
Contributor

@dspavlov dspavlov May 3, 2018

Choose a reason for hiding this comment

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

Looks like has next called twice will change iterarot state

while(state(next) == null && next < size)
next++;
idx = states.nextSetBit(idx);

return next < size;
return idx != -1;
}

@Override public Entry<Integer, GridDhtPartitionState> next() {
if (!hasNext())
throw new NoSuchElementException();

cur = next;
next++;
cur = idx / BITS;
Copy link
Contributor

@dspavlov dspavlov May 3, 2018

Choose a reason for hiding this comment

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

I suggest to document how it works. It is not totally clear to me after reading the code what is done below


int bitN = idx % BITS;

int st = 1 << bitN;

for (int i = 1; i < BITS - bitN; i++)
st |= (states.get(idx + i) ? 1 : 0) << i + bitN;

final int ordinal = st - 1;

idx += (BITS - bitN);

return new Entry<Integer, GridDhtPartitionState>() {
int p = cur;
int state = ordinal;

@Override public Integer getKey() {
return p;
}

@Override public GridDhtPartitionState getValue() {
return state(p);
return GridDhtPartitionState.fromOrdinal(state);
}

@Override public GridDhtPartitionState setValue(GridDhtPartitionState val) {
Expand Down Expand Up @@ -219,4 +227,4 @@ private GridDhtPartitionState state(int part) {
@Override public int hashCode() {
return 31 * states.hashCode() + size;
}
}
}
26 changes: 25 additions & 1 deletion modules/core/src/test/java/org/apache/ignite/util/GridPartitionMapSelfTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,16 @@

import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import org.apache.ignite.internal.processors.cache.distributed.dht.GridDhtPartitionState;
import org.apache.ignite.internal.util.GridPartitionStateMap;
import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
import org.apache.ignite.testframework.junits.common.GridCommonTest;

import static org.junit.Assert.assertNotEquals;

/**
* Grid utils tests.
*/
Expand Down Expand Up @@ -144,6 +147,27 @@ public void testCopyNoActive() {
assertEquals(1, cp2.size());
}

/**
*
Copy link
Contributor

@dspavlov dspavlov May 3, 2018

Choose a reason for hiding this comment

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

Could you please document what test does and what is expected.

*/
public void testIteratorNext() {
GridPartitionStateMap map = new GridPartitionStateMap();

initMap(map);

Iterator<Map.Entry<Integer, GridDhtPartitionState>> iter = map.entrySet().iterator();

Map.Entry<Integer, GridDhtPartitionState> entry1 = iter.next();
Map.Entry<Integer, GridDhtPartitionState> entry2 = iter.next();

iter.remove();

assertNotNull(entry1.getValue());
assertNotNull(entry2.getValue());
assertNotEquals(entry1.getKey(), entry2.getKey());
Copy link
Contributor

@dspavlov dspavlov May 3, 2018

Choose a reason for hiding this comment

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

I suggest to add space before sematic block

assertNotEquals(entry1.getValue(), entry2.getValue());
}

/** */
private GridPartitionStateMap initMap(GridPartitionStateMap map) {
map.put(0, GridDhtPartitionState.MOVING);
Expand All @@ -159,4 +183,4 @@ private GridPartitionStateMap initMap(GridPartitionStateMap map) {

return map;
}
}
}