Tree inserts: prefer read lock than write lock

[jschmieg]

Modification in tree locking algorithm during inserts and reads. This algorithm prefers read-locks and then write-lock for inserts.

---

This change is 

[KFilipek]

Reviewed 3 of 6 files at r1.
Review status: 3 of 6 files reviewed at latest revision, 23 unresolved discussions.

---

src/pmse_index_cursor.cpp, line 70 at r1 (raw file):

bool PmseCursor::lower_bound(IndexKeyEntry entry, CursorObject& cursor, std::list<nvml::obj::shared_mutex *>& locks) {
    uint64_t i = 0;
    int64_t cmp;

Scope of variables should be reduced

---

src/pmse_index_cursor.cpp, line 72 at r1 (raw file):

    int64_t cmp;
    persistent_ptr<PmseTreeNode> current = _tree->_root;
    (current->_pmutex).lock_shared();

Unnecessary parentheses

---

src/pmse_index_cursor.cpp, line 78 at r1 (raw file):

IndexEntryComparison c(Ordering::make(_ordering));

Should be swapped with IndexKeyEntry for better code understanding:

auto iKeyEntry = IndexKeyEntry(current->keys[i].getBSON(), RecordId(current->keys[i].loc));
auto cmp = IndexEntryComparison(Ordering::make(_ordering)).compare(entry, iKeyEntry);

---

src/pmse_index_cursor.cpp, line 90 at r1 (raw file):

        current = child;
    }
    locks.push_back(&(current->_pmutex));

Unnecessary parentheses

---

src/pmse_index_cursor.cpp, line 104 at r1 (raw file):

_locateFoundDataEnd = true;
return false;

Obfuscating indentation, should be in else block

---

src/pmse_index_cursor.cpp, line 116 at r1 (raw file):

    if (!_endState)
        return false;
    int cmp;

To one line, remove parentheses

---

src/pmse_index_cursor.cpp, line 118 at r1 (raw file):

    int cmp;
    cmp = (_cursor.node->keys[_cursor.index]).getBSON().woCompare(_endState->query.key, _ordering, false);
    if (cmp == 0) {

Related issue: #118
Code multiplicated few times

---

src/pmse_index_cursor.cpp, line 160 at r1 (raw file):
Comparator as above.

Consider:

if (cmp == 0) {
if ((_cursor.node->keys[_cursor.index]).loc < query.loc.repr()) {
cmp = -1;
} else if ((_cursor.node->keys[_cursor.index]).loc > query.loc.repr()) {
cmp = 1;
} else {
cmp = 0;
}
}
if (cmp) {
moveToNext();

versus

if (cmp == 0) {
if (_cursor.node->keys[_cursor.index].loc < query.loc.repr() || _cursor.node->keys[_cursor.index].loc > query.loc.repr()) {
moveToNext();
}
}

What's the difference?

---

src/pmse_index_cursor.cpp, line 323 at r1 (raw file):

void PmseCursor::unlockTree(std::list<nvml::obj::shared_mutex *>& locks) {
    std::list<nvml::obj::shared_mutex *>::const_iterator iterator;

no need tu put spaces between * and type

---

src/pmse_index_cursor.cpp, line 329 at r1 (raw file):

        }
        locks.erase(locks.begin(), locks.end());
    }catch(std::exception &e) {}

missing spaces between } ( etc.

---

src/pmse_index_cursor.h, line 102 at r1 (raw file):

    void seekEndCursor();
    bool lower_bound(IndexKeyEntry entry, CursorObject& cursor, std::list<nvml::obj::shared_mutex *>& locks);
    bool previous(CursorObject&);

Only declared

---

src/pmse_index_cursor.h, line 120 at r1 (raw file):

Quoted 4 lines of code…

> const bool _unique; > CursorObject _returnValue; > static IndexKeyEntry_PM min; > static IndexKeyEntry_PM max;

Not used

---

src/pmse_index_cursor.h, line 122 at r1 (raw file):

struct EndState {

Structure wrapping IndexKeyEntry without any reason

---

src/pmse_index_cursor.h, line 131 at r1 (raw file):

    bool _endPositionIsDataEnd;
    bool _locateFoundDataEnd;
    bool _wasMoved;

Not used variable

---

src/pmse_tree.cpp, line 508 at r1 (raw file):

    if (current == nullptr)
            return current;

Why not: return nullptr?

---

src/pmse_tree.cpp, line 511 at r1 (raw file):

    if(current->is_leaf){
        (current->_pmutex).lock();

Parentheses

---

src/pmse_tree.cpp, line 512 at r1 (raw file):

    if(current->is_leaf){
        (current->_pmutex).lock();
        if(current.raw_ptr()->off != _root.raw_ptr()->off)

spaces after if, brackets in the same line

---

src/pmse_tree.cpp, line 517 at r1 (raw file):

}
else{

?

---

src/pmse_tree.cpp, line 523 at r1 (raw file):

    }
    (current->_pmutex).lock_shared();
    if(current.raw_ptr()->off != _root.raw_ptr()->off)

Equality operator is overloaded for persistent_ptr<>

---

src/pmse_tree.cpp, line 550 at r1 (raw file):

    current->parent->_pmutex.unlock_shared();
    }
    if (!nodeIsSafeForOperation(current, insert)){

This block should be rewrote

---

src/pmse_tree.cpp, line 568 at r1 (raw file):

        locks.push_back(&(current->_pmutex));
        if (current == nullptr)
            return current;

return nullptr;

---

src/pmse_tree.cpp, line 570 at r1 (raw file):

            return current;

        while (!current->is_leaf) {

Code duplication with line 529

---

src/pmse_tree.cpp, line 919 at r1 (raw file):

     */
    if (node->num_keys < (TREE_ORDER)) {

remove empty line

---

Comments from Reviewable

[KFilipek]

Reviewed 3 of 6 files at r1.
Review status: all files reviewed at latest revision, 23 unresolved discussions.

---

Comments from Reviewable

[jschmieg]

Review status: all files reviewed at latest revision, 23 unresolved discussions.

---

src/pmse_index_cursor.cpp, line 70 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Scope of variables should be reduced

If you mean scope, then it is OK. It is local variable.

---

src/pmse_index_cursor.cpp, line 72 at r1 (raw file):

parentheses
I prefer them in this place for better readability.

---

src/pmse_index_cursor.cpp, line 78 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

IndexEntryComparison c(Ordering::make(_ordering));

Should be swapped with IndexKeyEntry for better code understanding:

auto iKeyEntry = IndexKeyEntry(current->keys[i].getBSON(), RecordId(current->keys[i].loc));
auto cmp = IndexEntryComparison(Ordering::make(_ordering)).compare(entry, iKeyEntry);

Looks the same for me.

---

src/pmse_index_cursor.cpp, line 90 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Unnecessary parentheses

Parenthesis here simplify my understanding whole behavior

---

src/pmse_index_cursor.cpp, line 104 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

_locateFoundDataEnd = true;
return false;

Obfuscating indentation, should be in else block

OK, but return was just line above.

---

src/pmse_index_cursor.cpp, line 116 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

To one line, remove parentheses

Parenthesis here simplify my understanding whole behavior

---

src/pmse_tree.cpp, line 511 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Parentheses

Parenthesis here simplify my understanding whole behavior

---

src/pmse_tree.cpp, line 517 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

}
else{

?

?

---

src/pmse_tree.cpp, line 550 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

This block should be rewrote

Tree split won't happen more than 2 times.

---

Comments from Reviewable

[jschmieg]

Review status: all files reviewed at latest revision, 23 unresolved discussions.

---

src/pmse_index_cursor.h, line 122 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

struct EndState {

Structure wrapping IndexKeyEntry without any reason

Query is used in many places. Check also similar definition in other engines.

---

Comments from Reviewable

[KFilipek]

Reviewed 1 of 3 files at r2.
Review status: 3 of 6 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

---

src/pmse_index_cursor.h, line 122 at r1 (raw file):
I had in mind nested classes only for change name.

typedef IndexKeyEntry EndState;

do the job.
Then in code:

_endState->query.loc.repr()

is changed to:

_endState->loc.repr()

and:
_endState->query
to
_endState.get()

---

src/pmse_tree.cpp, line 517 at r1 (raw file):

Previously, jschmieg wrote…

?

} else {

---

src/pmse_tree.cpp, line 550 at r1 (raw file):

Previously, jschmieg wrote…

Tree split won't happen more than 2 times.

so:
for(int i = 0; i < 2; i++) {
code we want to run two times
}

---

Comments from Reviewable

[jschmieg]

Review status: 3 of 6 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

---

src/pmse_tree.cpp, line 550 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

so:
for(int i = 0; i < 2; i++) {
code we want to run two times
}

not 2 but 0,1 or 2.

---

Comments from Reviewable

[jschmieg]

Review status: 3 of 6 files reviewed at latest revision, 23 unresolved discussions.

---

src/pmse_index_cursor.cpp, line 72 at r1 (raw file):

Previously, jschmieg wrote…

parentheses
I prefer them in this place for better readability.

Done.

---

src/pmse_index_cursor.cpp, line 78 at r1 (raw file):

Previously, jschmieg wrote…

Looks the same for me.

Done.

---

src/pmse_index_cursor.cpp, line 90 at r1 (raw file):

Previously, jschmieg wrote…

Parenthesis here simplify my understanding whole behavior

Done.

---

src/pmse_index_cursor.cpp, line 104 at r1 (raw file):

Previously, jschmieg wrote…

OK, but return was just line above.

Done.

---

src/pmse_index_cursor.cpp, line 323 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

no need tu put spaces between * and type

Done.

---

src/pmse_index_cursor.cpp, line 329 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

missing spaces between } ( etc.

Done.

---

src/pmse_index_cursor.h, line 102 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Only declared

?

---

src/pmse_index_cursor.h, line 120 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

const bool _unique;
CursorObject _returnValue;
static IndexKeyEntry_PM min;
static IndexKeyEntry_PM max;

Not used

Done.

---

src/pmse_index_cursor.h, line 122 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

I had in mind nested classes only for change name.

typedef IndexKeyEntry EndState;

do the job.
Then in code:

_endState->query.loc.repr()

is changed to:

_endState->loc.repr()

and:
_endState->query
to
_endState.get()

Done.

---

src/pmse_index_cursor.h, line 131 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Not used variable

Done.

---

src/pmse_tree.cpp, line 508 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Why not: return nullptr?

Done.

---

src/pmse_tree.cpp, line 511 at r1 (raw file):

Previously, jschmieg wrote…

Parenthesis here simplify my understanding whole behavior

Done.

---

src/pmse_tree.cpp, line 512 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

spaces after if, brackets in the same line

Done.

---

src/pmse_tree.cpp, line 517 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

} else {

Done.

---

src/pmse_tree.cpp, line 550 at r1 (raw file):

Previously, jschmieg wrote…

not 2 but 0,1 or 2.

Done.

---

src/pmse_tree.cpp, line 568 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

return nullptr;

Done.

---

src/pmse_tree.cpp, line 919 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

remove empty line

Done.

---

Comments from Reviewable

[jschmieg]

Review status: 2 of 6 files reviewed at latest revision, 23 unresolved discussions.

---

src/pmse_tree.cpp, line 523 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Equality operator is overloaded for persistent_ptr<>

Done.

---

Comments from Reviewable

[jschmieg]

Review status: 2 of 6 files reviewed at latest revision, 23 unresolved discussions.

---

src/pmse_index_cursor.cpp, line 118 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Related issue: #118
Code multiplicated few times

created US.

---

src/pmse_index_cursor.cpp, line 160 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Comparator as above.

Consider:

if (cmp == 0) {
if ((_cursor.node->keys[_cursor.index]).loc < query.loc.repr()) {
cmp = -1;
} else if ((_cursor.node->keys[_cursor.index]).loc > query.loc.repr()) {
cmp = 1;
} else {
cmp = 0;
}
}
if (cmp) {
moveToNext();

versus

if (cmp == 0) {
if (_cursor.node->keys[_cursor.index].loc < query.loc.repr() || _cursor.node->keys[_cursor.index].loc > query.loc.repr()) {
moveToNext();
}
}

What's the difference?

created US

---

src/pmse_tree.cpp, line 570 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Code duplication with line 529

created US.

---

Comments from Reviewable

[krzycz]

Reviewed 2 of 6 files at r1, 4 of 4 files at r5.
Review status: all files reviewed at latest revision, 23 unresolved discussions.

---

Comments from Reviewable

[KFilipek]

---

Reviewed 1 of 2 files at r3, 3 of 4 files at r5.
Review status: 5 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable