Skip to content

Prefer ArrayList over LinkedList and native array #1334

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
belugabehr wants to merge 2 commits into from
Closed

Prefer ArrayList over LinkedList and native array #1334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3846145
Prefer ArrayList over LinkedList and native array
belugabehr Aug 23, 2019
1c01ca1
Made changes based on code review
belugabehr Sep 11, 2019
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
70 changes: 34 additions & 36 deletions core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,12 +1132,12 @@ public static class Reader extends HeapIterator implements FileSKVIterator {

private final CachableBlockFile.Reader reader;

private final ArrayList<LocalityGroupMetadata> localityGroups = new ArrayList<>();
private final ArrayList<LocalityGroupMetadata> sampleGroups = new ArrayList<>();
private final List<LocalityGroupMetadata> localityGroups = new ArrayList<>();
private final List<LocalityGroupMetadata> sampleGroups = new ArrayList<>();

private final LocalityGroupReader[] currentReaders;
private final LocalityGroupReader[] readers;
private final LocalityGroupReader[] sampleReaders;
private final List<LocalityGroupReader> currentReaders;
private final List<LocalityGroupReader> readers;
private final List<LocalityGroupReader> sampleReaders;
private final LocalityGroupContext lgContext;
private LocalityGroupSeekCache lgCache;

Expand Down Expand Up @@ -1166,29 +1166,27 @@ public Reader(CachableBlockFile.Reader rdr) throws IOException {
throw new IOException("Did not see expected version, saw " + ver);

int size = mb.readInt();
currentReaders = new LocalityGroupReader[size];
currentReaders = new ArrayList<>(size);

deepCopies = new LinkedList<>();

for (int i = 0; i < size; i++) {
LocalityGroupMetadata lgm = new LocalityGroupMetadata(ver, rdr);
lgm.readFields(mb);
localityGroups.add(lgm);

currentReaders[i] = new LocalityGroupReader(reader, lgm, ver);
currentReaders.add(new LocalityGroupReader(reader, lgm, ver));
}

readers = currentReaders;

if (ver == RINDEX_VER_8 && mb.readBoolean()) {
sampleReaders = new LocalityGroupReader[size];
sampleReaders = new ArrayList<>(size);

for (int i = 0; i < size; i++) {
LocalityGroupMetadata lgm = new LocalityGroupMetadata(ver, rdr);
lgm.readFields(mb);
sampleGroups.add(lgm);

sampleReaders[i] = new LocalityGroupReader(reader, lgm, ver);
sampleReaders.add(new LocalityGroupReader(reader, lgm, ver));
}

samplerConfig = new SamplerConfigurationImpl(mb);
Expand All @@ -1203,46 +1201,42 @@ public Reader(CachableBlockFile.Reader rdr) throws IOException {

lgContext = new LocalityGroupContext(currentReaders);

createHeap(currentReaders.length);
createHeap(currentReaders.size());
}

private Reader(Reader r, LocalityGroupReader[] sampleReaders) {
super(sampleReaders.length);
private Reader(Reader r, List<LocalityGroupReader> sampleReaders) {
super(sampleReaders.size());
this.reader = r.reader;
this.currentReaders = new LocalityGroupReader[sampleReaders.length];
this.currentReaders = new ArrayList<>(sampleReaders.size());
this.deepCopies = r.deepCopies;
this.deepCopy = false;
this.readers = r.readers;
this.sampleReaders = r.sampleReaders;
this.samplerConfig = r.samplerConfig;
this.rfileVersion = r.rfileVersion;
for (int i = 0; i < sampleReaders.length; i++) {
this.currentReaders[i] = sampleReaders[i];
this.currentReaders[i].setInterruptFlag(r.interruptFlag);
for (LocalityGroupReader reader : sampleReaders) {
reader.setInterruptFlag(r.interruptFlag);
this.currentReaders.add(reader);
}
this.lgContext = new LocalityGroupContext(currentReaders);
}

private Reader(Reader r, boolean useSample) {
super(r.currentReaders.length);
super(r.currentReaders.size());
this.reader = r.reader;
this.currentReaders = new LocalityGroupReader[r.currentReaders.length];
this.currentReaders = new ArrayList<>(r.currentReaders.size());
this.deepCopies = r.deepCopies;
this.deepCopy = true;
this.samplerConfig = r.samplerConfig;
this.rfileVersion = r.rfileVersion;
this.readers = r.readers;
this.sampleReaders = r.sampleReaders;

for (int i = 0; i < r.readers.length; i++) {
if (useSample) {
this.currentReaders[i] = new LocalityGroupReader(r.sampleReaders[i]);
this.currentReaders[i].setInterruptFlag(r.interruptFlag);
} else {
this.currentReaders[i] = new LocalityGroupReader(r.readers[i]);
this.currentReaders[i].setInterruptFlag(r.interruptFlag);
}

List<LocalityGroupReader> readers = (useSample) ? r.sampleReaders : r.readers;
Copy link
Member

@ctubbsii ctubbsii Aug 26, 2019

Choose a reason for hiding this comment

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

This is a nice improvement to simplify the following loop.

for (LocalityGroupReader reader : readers) {
LocalityGroupReader newReader = new LocalityGroupReader(reader);
Copy link
Member

@ctubbsii ctubbsii Aug 26, 2019

Choose a reason for hiding this comment

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

Good opportunity to use var if one were inclined.

Suggested change
LocalityGroupReader newReader = new LocalityGroupReader(reader);
var newReader = new LocalityGroupReader(reader);

newReader.setInterruptFlag(r.interruptFlag);
this.currentReaders.add(newReader);
}
this.lgContext = new LocalityGroupContext(currentReaders);
}
Expand All @@ -1263,11 +1257,13 @@ private void closeLocalityGroupReaders() {

@Override
public void closeDeepCopies() {
if (deepCopy)
if (deepCopy) {
throw new RuntimeException("Calling closeDeepCopies on a deep copy is not supported");
}

for (Reader deepCopy : deepCopies)
for (Reader deepCopy : deepCopies) {
deepCopy.closeLocalityGroupReaders();
}

deepCopies.clear();
}
Expand Down Expand Up @@ -1301,7 +1297,7 @@ public void close() throws IOException {

@Override
public Key getFirstKey() throws IOException {
if (currentReaders.length == 0) {
if (currentReaders.isEmpty()) {
return null;
}

Expand All @@ -1322,7 +1318,7 @@ public Key getFirstKey() throws IOException {

@Override
public Key getLastKey() throws IOException {
if (currentReaders.length == 0) {
if (currentReaders.isEmpty()) {
return null;
}

Expand Down Expand Up @@ -1398,7 +1394,7 @@ public Map<String,ArrayList<ByteSequence>> getLocalityGroupCF() {
if (lcg.columnFamilies == null) {
Preconditions.checkState(lcg.isDefaultLG, "Group %s has null families. "
+ "Only expect default locality group to have null families.", lcg.name);
setCF = new ArrayList<>();
setCF = new ArrayList<>(0);
} else {
setCF = new ArrayList<>(lcg.columnFamilies.keySet());
}
Expand Down Expand Up @@ -1497,11 +1493,13 @@ public void printInfo(boolean includeIndexDetails) throws IOException {

@Override
public void setInterruptFlag(AtomicBoolean flag) {
if (deepCopy)
if (deepCopy) {
throw new RuntimeException("Calling setInterruptFlag on a deep copy is not supported");
}

if (deepCopies.size() != 0)
if (!deepCopies.isEmpty()) {
Copy link
Member

@ctubbsii ctubbsii Aug 26, 2019

Choose a reason for hiding this comment

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

Unrelated changes, but nice changes to see anyway. I sometimes have my IDE automatically add missing braces for single-statement blocks, but the downside is that they can clutter the review (same with the unrelated change to convert this to isEmpty()). I don't think there' a problem here... the changes in this PR are simple enough to understand, and these are nice changes to see.

throw new RuntimeException("Setting interrupt flag after calling deep copy not supported");
}

setInterruptFlagInternal(flag);
}
Expand Down
6 changes: 3 additions & 3 deletions core/src/main/java/org/apache/accumulo/core/iterators/system/LocalityGroupIterator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ public static class LocalityGroupContext {
final LocalityGroup defaultGroup;
final Map<ByteSequence,LocalityGroup> groupByCf;

public LocalityGroupContext(LocalityGroup[] groups) {
this.groups = Collections.unmodifiableList(Arrays.asList(groups));
public LocalityGroupContext(final List<? extends LocalityGroup> groups) {
this.groups = Collections.unmodifiableList(groups);
this.groupByCf = new HashMap<>();
LocalityGroup foundDefault = null;

Expand Down Expand Up @@ -129,7 +129,7 @@ public int getNumLGSeeked() {

public LocalityGroupIterator(LocalityGroup[] groups) {
super(groups.length);
this.lgContext = new LocalityGroupContext(groups);
this.lgContext = new LocalityGroupContext(Arrays.asList(groups));
Copy link
Member

@ctubbsii ctubbsii Sep 11, 2019

Choose a reason for hiding this comment

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

Since this system iterator is also not public API, I wonder if there's something we can do here to pass this in as a list initially, rather than as an array that we have to convert. This could also be done in subsequent work. What do you think, @belugabehr ? Is it worth going down the rabbit hole a bit further on this one, or doing in a future change?

}

@Override
Expand Down