Skip to content

perf(rocksdb): RocksDBStore remove redundant checkOpened() call #2863

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

Merged
VGalaxies merged 1 commit into from
Sep 16, 2025
Merged

perf(rocksdb): RocksDBStore remove redundant checkOpened() call #2863

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
87 changes: 47 additions & 40 deletions ...egraph-rocksdb/src/main/java/org/apache/hugegraph/backend/store/rocksdb/RocksDBStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,6 @@ public void mutate(BackendMutation mutation) {
Lock readLock = this.storeLock.readLock();
Copy link
Copy Markdown
Member

@imbajin imbajin Sep 1, 2025

Choose a reason for hiding this comment

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

Good performance optimization removing redundant checkOpened() calls. Ensure comprehensive testing covers store state consistency under concurrent access and that the performance gains are measurable without introducing subtle bugs.

readLock.lock();
try {
this.checkOpened();
if (LOG.isDebugEnabled()) {
LOG.debug("Store {} mutation: {}", this.store, mutation);
}
Expand Down Expand Up @@ -490,7 +489,6 @@ public Iterator<BackendEntry> query(Query query) {
readLock.lock();

try {
this.checkOpened();
HugeType tableType = RocksDBTable.tableType(query);
RocksDBTable table;
RocksDBSessions.Session session;
Expand Down Expand Up @@ -525,7 +523,6 @@ public Number queryNumber(Query query) {
Lock readLock = this.storeLock.readLock();
readLock.lock();
try {
this.checkOpened();
HugeType tableType = RocksDBTable.tableType(query);
RocksDBTable table = this.table(tableType);
return table.queryNumber(this.session(tableType), query);
Expand Down Expand Up @@ -648,8 +645,6 @@ public void beginTx() {
Lock readLock = this.storeLock.readLock();
readLock.lock();
try {
this.checkOpened();

for (RocksDBSessions.Session session : this.session()) {
assert !session.hasChanges();
}
Expand All @@ -663,7 +658,6 @@ public void commitTx() {
Lock readLock = this.storeLock.readLock();
readLock.lock();
try {
this.checkOpened();
// Unable to guarantee atomicity when committing multi sessions
for (RocksDBSessions.Session session : this.session()) {
Object count = session.commit();
Expand All @@ -681,8 +675,6 @@ public void rollbackTx() {
Lock readLock = this.storeLock.readLock();
readLock.lock();
try {
this.checkOpened();

for (RocksDBSessions.Session session : this.session()) {
session.rollback();
}
Expand All @@ -691,19 +683,6 @@ public void rollbackTx() {
}
}

@Override
protected RocksDBSessions.Session session(HugeType tableType) {
this.checkOpened();

// Optimized disk
String disk = this.tableDiskMapping.get(tableType);
if (disk != null) {
return this.db(disk).session();
}

return this.sessions.session();
}

@Override
public Map<String, String> createSnapshot(String snapshotPrefix) {
Lock readLock = this.storeLock.readLock();
Expand Down Expand Up @@ -785,22 +764,6 @@ private void useSessions() {
}
}

private List<RocksDBSessions.Session> session() {
this.checkOpened();

if (this.tableDiskMapping.isEmpty()) {
return Collections.singletonList(this.sessions.session());
}

// Collect session of each table with optimized disk
List<RocksDBSessions.Session> list = new ArrayList<>(this.tableDiskMapping.size() + 1);
list.add(this.sessions.session());
for (String disk : this.tableDiskMapping.values()) {
list.add(db(disk).session());
}
return list;
}

private void closeSessions() {
Iterator<Map.Entry<String, RocksDBSessions>> iter = this.dbs.entrySet().iterator();
while (iter.hasNext()) {
Expand All @@ -813,12 +776,56 @@ private void closeSessions() {
}
}

private Collection<RocksDBSessions> sessions() {
private final Collection<RocksDBSessions> sessions() {
return this.dbs.values();
}

private void parseTableDiskMapping(Map<String, String> disks, String dataPath) {
private final List<RocksDBSessions.Session> session() {
Copy link
Copy Markdown
Member

@imbajin imbajin Sep 1, 2025

Choose a reason for hiding this comment

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

Consider making session() method final like sessions() for consistency and to prevent accidental overrides that could break the optimized session management logic.

this.checkDbOpened();

// Collect session of standard disk
RocksDBSessions.Session session = this.sessions.session();
if (!session.opened()) {
this.checkOpened();
}

if (this.tableDiskMapping.isEmpty()) {
return ImmutableList.of(session);
}

Copy link
Copy Markdown
Member

@imbajin imbajin Sep 1, 2025

Choose a reason for hiding this comment

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

Performance optimization looks good, but consider session state consistency. The conditional checkOpened() call based on session.opened() could introduce race conditions between the check and actual session usage.

// Collect session of each table with optimized disk
List<RocksDBSessions.Session> list = new ArrayList<>(this.tableDiskMapping.size() + 1);
list.add(session);
for (String disk : this.tableDiskMapping.values()) {
RocksDBSessions.Session optimizedSession = this.db(disk).session();
assert optimizedSession.opened();
Copy link
Copy Markdown
Member

@imbajin imbajin Sep 1, 2025

Choose a reason for hiding this comment

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

Inconsistent session state handling: optimized sessions use assert optimizedSession.opened() while standard sessions get conditional checks. Consider applying the same defensive pattern to both session types for consistency.

Copy link
Copy Markdown
Contributor Author

@javeme javeme Sep 7, 2025

Choose a reason for hiding this comment

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

It is believed here that optimizedSession must be the opened state, otherwise it is a bug in logic. We consider this optimizedSession state to be consistent with the main session state.

list.add(optimizedSession);
}
return list;
}

@Override
protected RocksDBSessions.Session session(HugeType tableType) {
this.checkDbOpened();

RocksDBSessions.Session session;
String disk = this.tableDiskMapping.get(tableType);
if (disk != null) {
// Optimized disk
session = this.db(disk).session();
} else {
// Standard disk
session = this.sessions.session();
}

if (!session.opened()) {
this.checkOpened();
}
return session;
}

private void parseTableDiskMapping(Map<String, String> disks, String dataPath) {
// reset and parse
this.tableDiskMapping.clear();
for (Map.Entry<String, String> disk : disks.entrySet()) {
// The format of `disk` like: `graph/vertex: /path/to/disk1`
Expand Down Expand Up @@ -869,7 +876,7 @@ private void checkDbOpened() {
}

protected RocksDBSessions db(HugeType tableType) {
this.checkOpened();
this.checkDbOpened();

// Optimized disk
String disk = this.tableDiskMapping.get(tableType);
Expand Down
Loading