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

DeletedMemoryAccessException is Exception #210

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

DeletedMemoryAccessException is Exception #210

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ccd7b5d
phase 1: runtimeexception to exception
li0nr Apr 20, 2022
486e578
fully changed runtimeexceptimeexception to exception
li0nr Apr 20, 2022
b840c75
addressing some of review comments
li0nr Jun 29, 2022
57047d2
resolving issue with iterator
li0nr Sep 18, 2022
7343569
fixing validate stage syntax error
li0nr Sep 18, 2022
8209e13
minor bug fix
li0nr Sep 18, 2022
192a236
adressing core review, adding some comments for some of the cases (re…
li0nr Oct 11, 2022
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
2 changes: 1 addition & 1 deletion core/src/main/java/com/yahoo/oak/BasicChunk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ protected void updateBasicChild(BasicChunk<K, V> child) {

abstract boolean readValueFromEntryIndex(ValueBuffer value, int ei);

abstract void lookUp(ThreadContext ctx, K key);
abstract void lookUp(ThreadContext ctx, K key) throws DeletedMemoryAccessException;
li0nr marked this conversation as resolved.
Show resolved Hide resolved

/*-------------- Publishing related methods and getters ---------------*/
/**
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/com/yahoo/oak/DeletedMemoryAccessException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

package com.yahoo.oak;

public class DeletedMemoryAccessException extends RuntimeException {
public class DeletedMemoryAccessException extends Exception {
li0nr marked this conversation as resolved.
Show resolved Hide resolved
public DeletedMemoryAccessException() {
super();
}
Expand Down
6 changes: 3 additions & 3 deletions core/src/main/java/com/yahoo/oak/EntryArray.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,9 @@ int getNextNonZeroIndex(int currentIndex) {
int nextIndex = currentIndex;
while (!isNotZero && isIndexInBound(++nextIndex)) {

isNotZero = getEntryFieldLong(nextIndex, KEY_REF_OFFSET) != 0 ||
getEntryFieldLong(nextIndex, VALUE_REF_OFFSET) != 0 ||
getEntryFieldLong(nextIndex, OPT_FIELD_OFFSET) != 0;
isNotZero = array.getEntryFieldLong(nextIndex, KEY_REF_OFFSET) != 0 ||
sanastas marked this conversation as resolved.
Show resolved Hide resolved
array.getEntryFieldLong(nextIndex, VALUE_REF_OFFSET) != 0 ||
array.getEntryFieldLong(nextIndex, OPT_FIELD_OFFSET) != 0;
}
if (isIndexInBound(nextIndex)) {
return nextIndex;
Expand Down
47 changes: 30 additions & 17 deletions core/src/main/java/com/yahoo/oak/InternalOakBasics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,11 @@ V replace(K key, V value, OakTransformer<V> valueDeserializeTransformer) {

for (int i = 0; i < MAX_RETRIES; i++) {
BasicChunk<K, V> chunk = findChunk(key, ctx); // find orderedChunk matching key
chunk.lookUp(ctx, key);
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment above speaks about orderedChunk, do we know it is not going to happen for hash?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add an explanatory comment here?

chunk.lookUp(ctx, key);
} catch (DeletedMemoryAccessException e) {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an assert that chunk is not hashChunk? As a sanity check...

Copy link
Collaborator Author

@li0nr li0nr Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this opened to be discussed

}
if (!ctx.isValueValid()) {
return null;
}
Expand All @@ -184,7 +188,8 @@ V replace(K key, V value, OakTransformer<V> valueDeserializeTransformer) {
throw new RuntimeException("replace failed: reached retry limit (1024).");
}

boolean replace(K key, V oldValue, V newValue, OakTransformer<V> valueDeserializeTransformer) {
boolean replace(K key, V oldValue, V newValue, OakTransformer<V> valueDeserializeTransformer)
throws DeletedMemoryAccessException {
li0nr marked this conversation as resolved.
Show resolved Hide resolved
ThreadContext ctx = getThreadContext();

for (int i = 0; i < MAX_RETRIES; i++) {
Expand Down Expand Up @@ -355,7 +360,7 @@ public boolean hasNext() {
return (state != null);
}

protected abstract void initAfterRebalance();
protected abstract void initAfterRebalance() throws DeletedMemoryAccessException;
li0nr marked this conversation as resolved.
Show resolved Hide resolved

// the actual next()
public abstract T next();
Expand Down Expand Up @@ -390,7 +395,7 @@ public void remove() {
* The first long is the key's reference, the integer is the value's version and the second long is
* the value's reference. If {@code needsValue == false}, then the value of the map entry is {@code null}.
*/
void advance(boolean needsValue) {
void advance(boolean needsValue) throws DeletedMemoryAccessException {
boolean validState = false;

while (!validState) {
Expand Down Expand Up @@ -471,7 +476,8 @@ protected void invalidatePrevState() {

protected abstract BasicChunk<K, V> getNextChunk(BasicChunk<K, V> current);

protected abstract BasicChunk.BasicChunkIter getChunkIter(BasicChunk<K, V> current);
protected abstract BasicChunk.BasicChunkIter getChunkIter(BasicChunk<K, V> current)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to explain in which cases the exception is thrown. As I remember we were asking ourselves as well. Please add a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to be discussed

throws DeletedMemoryAccessException;

/**
* advance state to the new position
Expand All @@ -483,21 +489,28 @@ protected boolean advanceState() {
BasicChunk.BasicChunkIter chunkIter = getState().getChunkIter();

updatePreviousState();

while (!chunkIter.hasNext()) { // skip empty chunks
chunk = getNextChunk(chunk);
if (chunk == null) {
//End of iteration
setState(null);
return false;

for (int i = 0; i < MAX_RETRIES; i++) {
while (!chunkIter.hasNext()) { // skip empty chunks
chunk = getNextChunk(chunk);
if (chunk == null) {
//End of iteration
setState(null);
return false;
}
try {
chunkIter = getChunkIter(chunk);
} catch (DeletedMemoryAccessException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment when getChunkIter() can return DeletedMemoryAccessException e

continue;
}
}
chunkIter = getChunkIter(chunk);
}

int nextIndex = chunkIter.next(ctx);
getState().set(chunk, chunkIter, nextIndex);
int nextIndex = chunkIter.next(ctx);
getState().set(chunk, chunkIter, nextIndex);

return true;
return true;
}
throw new RuntimeException("put failed: reached retry limit (1024).");
}

}
Expand Down
29 changes: 22 additions & 7 deletions core/src/main/java/com/yahoo/oak/InternalOakHash.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,11 @@ boolean replace(K key, V oldValue, V newValue, OakTransformer<V> valueDeserializ
for (int i = 0; i < MAX_RETRIES; i++) {
// find chunk matching key, puts this key hash into ctx.operationKeyHash
BasicChunk<K, V> c = findChunk(key, ctx);
c.lookUp(ctx, key);
try {
c.lookUp(ctx, key);
} catch (DeletedMemoryAccessException e) {
continue;
li0nr marked this conversation as resolved.
Show resolved Hide resolved
}
if (!ctx.isValueValid()) {
return false;
}
Expand Down Expand Up @@ -407,8 +411,11 @@ boolean computeIfPresent(K key, Consumer<OakScopedWriteBuffer> computer) {
for (int i = 0; i < MAX_RETRIES; i++) {
// find chunk matching key, puts this key hash into ctx.operationKeyHash
BasicChunk<K, V> c = findChunk(key, ctx);
c.lookUp(ctx, key);

try {
c.lookUp(ctx, key);
} catch (DeletedMemoryAccessException e) {
continue;
}
if (ctx.isValueValid()) {
ValueUtils.ValueResult res = config.valueOperator.compute(ctx.value, computer);
if (res == ValueUtils.ValueResult.TRUE) {
Expand Down Expand Up @@ -441,8 +448,11 @@ Result remove(K key, V oldValue, OakTransformer<V> transformer) {
for (int i = 0; i < MAX_RETRIES; i++) {
// find chunk matching key, puts this key hash into ctx.operationKeyHash
BasicChunk<K, V> c = findChunk(key, ctx);
c.lookUp(ctx, key);

try {
c.lookUp(ctx, key);
} catch (DeletedMemoryAccessException e) {
continue;
}
if (!ctx.isKeyValid()) {
// There is no such key. If we did logical deletion and someone else did the physical deletion,
// then the old value is saved in v. Otherwise v is (correctly) null
Expand Down Expand Up @@ -799,8 +809,9 @@ class EntryTransformIterator<T> extends HashIter<T> {
}

public T next() {
ValueUtils.ValueResult res;
advance(true);
ValueUtils.ValueResult res = ctx.value.s.preRead();
res = ctx.value.s.preRead();
if (res == ValueUtils.ValueResult.FALSE) {
return next();
} else if (res == ValueUtils.ValueResult.RETRY) {
Expand All @@ -817,7 +828,11 @@ public T next() {
new AbstractMap.SimpleEntry<>(ctx.key, ctx.value);

T transformation = transformer.apply(entry);
ctx.value.s.postRead();
try {
ctx.value.s.postRead();
} catch (DeletedMemoryAccessException e) {
return next();
}
return transformation;
}
}
Expand Down
112 changes: 66 additions & 46 deletions core/src/main/java/com/yahoo/oak/InternalOakMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,8 @@ private void updateIndexAndNormalize(

// returns false when restart is needed
// (if rebalance happened or another valid entry with same key was found)
private boolean allocateAndLinkEntry(OrderedChunk<K, V> c, ThreadContext ctx, K key, boolean isPutIfAbsent) {
private boolean allocateAndLinkEntry(OrderedChunk<K, V> c, ThreadContext ctx, K key, boolean isPutIfAbsent)
throws DeletedMemoryAccessException {
// There was no such key found, going to allocate a new key.
// EntryOrderedSet allocates the entry (holding the key) and ctx is going to be updated
// to be used by EntryOrderedSet's subsequent requests to write value
Expand Down Expand Up @@ -815,7 +816,7 @@ boolean replace(K key, V oldValue, V newValue, OakTransformer<V> valueDeserializ
throw new RuntimeException("replace failed: reached retry limit (1024).");
}

Map.Entry<K, V> lowerEntry(K key) {
Map.Entry<K, V> lowerEntry(K key) throws DeletedMemoryAccessException {
Map.Entry<Object, OrderedChunk<K, V>> lowerChunkEntry = skiplist.lowerEntry(key);
if (lowerChunkEntry == null) {
/* we were looking for the minimal key */
Expand Down Expand Up @@ -913,7 +914,7 @@ abstract class OrderedIter<T> extends BasicIter<T> {
initState(isDescending, lo, loInclusive, hi, hiInclusive);
}

private boolean tooLow(OakScopedReadBuffer key) {
private boolean tooLow(OakScopedReadBuffer key) throws DeletedMemoryAccessException {
if (lo == null) {
return false;
}
Expand All @@ -922,7 +923,7 @@ private boolean tooLow(OakScopedReadBuffer key) {
return c > 0 || (c == 0 && !loInclusive);
}

private boolean tooHigh(OakScopedReadBuffer key) {
private boolean tooHigh(OakScopedReadBuffer key) throws DeletedMemoryAccessException {
if (hi == null) {
return false;
}
Expand All @@ -931,7 +932,7 @@ private boolean tooHigh(OakScopedReadBuffer key) {
}


private boolean inBounds(OakScopedReadBuffer key) {
private boolean inBounds(OakScopedReadBuffer key) throws DeletedMemoryAccessException {
if (!isDescending) {
return !tooHigh(key);
} else {
Expand Down Expand Up @@ -975,6 +976,7 @@ protected void initAfterRebalance() {
/**
* Advances next to higher entry.
* Return previous index
* @throws DeletedMemoryAccessException
li0nr marked this conversation as resolved.
Show resolved Hide resolved
*
*
*/
Expand Down Expand Up @@ -1067,43 +1069,51 @@ protected void initState(boolean isDescending, K lowerBound, boolean lowerInclus

OrderedChunk<K, V>.ChunkIter nextChunkIter;
OrderedChunk<K, V> nextOrderedChunk;

if (!isDescending) {
if (lowerBound != null) {
nextOrderedChunk = skiplist.floorEntry(lowerBound).getValue();
} else {
nextOrderedChunk = skiplist.firstEntry().getValue();
// need to iterate from the beginning of the orderedChunk till the end
}
if (nextOrderedChunk != null) {
OakScopedReadBuffer upperBoundKeyForChunk = getNextChunkMinKey(nextOrderedChunk);
nextChunkIter = lowerBound != null ?
nextOrderedChunk.ascendingIter(ctx, lowerBound, lowerInclusive, upperBound, upperInclusive,
upperBoundKeyForChunk)
: nextOrderedChunk
.ascendingIter(ctx, upperBound, upperInclusive, upperBoundKeyForChunk);
} else {
setState(null);
return;
}
} else {
nextOrderedChunk = upperBound != null ? skiplist.floorEntry(upperBound).getValue()
: skiplist.lastEntry().getValue();
if (nextOrderedChunk != null) {
nextChunkIter = upperBound != null ?
nextOrderedChunk
.descendingIter(ctx, upperBound, upperInclusive, lowerBound, lowerInclusive)
: nextOrderedChunk.descendingIter(ctx, lowerBound, lowerInclusive);
} else {
setState(null);
return;

for (int i = 0; i < MAX_RETRIES; i++) {
try {
if (!isDescending) {
if (lowerBound != null) {
nextOrderedChunk = skiplist.floorEntry(lowerBound).getValue();
} else {
nextOrderedChunk = skiplist.firstEntry().getValue();
// need to iterate from the beginning of the orderedChunk till the end
}
if (nextOrderedChunk != null) {
OakScopedReadBuffer upperBoundKeyForChunk = getNextChunkMinKey(nextOrderedChunk);
nextChunkIter = lowerBound != null ?
nextOrderedChunk.ascendingIter
(ctx, lowerBound, lowerInclusive, upperBound, upperInclusive, upperBoundKeyForChunk)
: nextOrderedChunk
.ascendingIter(ctx, upperBound, upperInclusive, upperBoundKeyForChunk);
} else {
setState(null);
return;
}
} else {
nextOrderedChunk = upperBound != null ? skiplist.floorEntry(upperBound).getValue()
: skiplist.lastEntry().getValue();
if (nextOrderedChunk != null) {
nextChunkIter = upperBound != null ?
nextOrderedChunk
.descendingIter(ctx, upperBound, upperInclusive, lowerBound, lowerInclusive)
: nextOrderedChunk.descendingIter(ctx, lowerBound, lowerInclusive);
} else {
setState(null);
return;
}
}

//Init state, not valid yet, must move forward
setPrevState(InternalOakMap.IteratorState.newInstance(null, null));
setState(IteratorState.newInstance(nextOrderedChunk, nextChunkIter));
advanceState();
} catch (DeletedMemoryAccessException e) {
li0nr marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
return;
}

//Init state, not valid yet, must move forward
setPrevState(InternalOakMap.IteratorState.newInstance(null, null));
setState(IteratorState.newInstance(nextOrderedChunk, nextChunkIter));
advanceState();
throw new RuntimeException("put failed: reached retry limit (1024).");
}

@Override
Expand All @@ -1121,7 +1131,7 @@ protected BasicChunk<K, V> getNextChunk(BasicChunk<K, V> current) {
}
}

protected BasicChunk.BasicChunkIter getChunkIter(BasicChunk<K, V> current) {
protected BasicChunk.BasicChunkIter getChunkIter(BasicChunk<K, V> current) throws DeletedMemoryAccessException {
Copy link
Contributor

Choose a reason for hiding this comment

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

minKeys are not released, right? From where the exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lets discuss

if (!isDescending) {
OakScopedReadBuffer upperBoundKeyForChunk = getNextChunkMinKey((OrderedChunk<K, V>) current);
return ((OrderedChunk<K, V>) current).ascendingIter(ctx, hi, hiInclusive, upperBoundKeyForChunk);
Expand Down Expand Up @@ -1167,9 +1177,13 @@ protected boolean advanceState() {
if (!chunk.readKeyFromEntryIndex(ctx.tempKey, nextIndex)) {
continue;
}
if (!inBounds(ctx.tempKey)) {
setState(null);
valueToReturn = false;
try {
if (!inBounds(ctx.tempKey)) {
setState(null);
valueToReturn = false;
}
} catch (DeletedMemoryAccessException e) {
continue;
}
}
}
Expand Down Expand Up @@ -1297,8 +1311,9 @@ class EntryTransformIterator<T> extends OrderedIter<T> {
}

public T next() {
ValueUtils.ValueResult res;
advance(true);
ValueUtils.ValueResult res = ctx.value.s.preRead();
res = ctx.value.s.preRead();
if (res == ValueUtils.ValueResult.FALSE) {
return next();
} else if (res == ValueUtils.ValueResult.RETRY) {
Expand All @@ -1315,7 +1330,12 @@ public T next() {
new AbstractMap.SimpleEntry<>(ctx.key, ctx.value);

T transformation = transformer.apply(entry);
ctx.value.s.postRead();
try {
ctx.value.s.postRead();
} catch (DeletedMemoryAccessException e) {
return next();
}

return transformation;
}
}
Expand Down
5 changes: 3 additions & 2 deletions core/src/main/java/com/yahoo/oak/NovaMMHeader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,13 @@ private static boolean atomicallySetDeleted(long headerAddress, long offHeapMeta
ValueUtils.ValueResult preRead(final int onHeapVersion, long headerAddress) {
long offHeapHeader = getOffHeapHeader(headerAddress);
if (RC.isReferenceDeleted(offHeapHeader)) {
throw new DeletedMemoryAccessException();
return ValueUtils.ValueResult.FALSE;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the contract of the method? Should be clearly defined. Let'd discuss whether to return false or to throw exception.

//throw new DeletedMemoryAccessException();
}
return ValueUtils.ValueResult.TRUE;
}

ValueUtils.ValueResult postRead(final int onHeapVersion, long headerAddress) {
ValueUtils.ValueResult postRead(final int onHeapVersion, long headerAddress) throws DeletedMemoryAccessException {
li0nr marked this conversation as resolved.
Show resolved Hide resolved
DirectUtils.UNSAFE.loadFence();
long offHeapHeader = getOffHeapHeader(headerAddress);
int offHeapVersion = RC.getFirst(offHeapHeader);
Expand Down
Loading