-
Notifications
You must be signed in to change notification settings - Fork 731
Make statistics optional #977
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -357,13 +357,15 @@ func (tx *Tx) close() { | |
| tx.db.rwlock.Unlock() | ||
|
|
||
| // Merge statistics. | ||
| tx.db.statlock.Lock() | ||
| tx.db.stats.FreePageN = freelistFreeN | ||
| tx.db.stats.PendingPageN = freelistPendingN | ||
| tx.db.stats.FreeAlloc = (freelistFreeN + freelistPendingN) * tx.db.pageSize | ||
| tx.db.stats.FreelistInuse = freelistAlloc | ||
| tx.db.stats.TxStats.add(&tx.stats) | ||
| tx.db.statlock.Unlock() | ||
| if tx.db.stats != nil { | ||
| tx.db.statlock.Lock() | ||
| tx.db.stats.FreePageN = freelistFreeN | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking if we can use atomic to update stats here. so no more lock and no more extra option for openning db.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think using atomic functions is problematic, since it only guarantees atomic of one operation, but it doesn't guarantee atomic of multiple operations. |
||
| tx.db.stats.PendingPageN = freelistPendingN | ||
| tx.db.stats.FreeAlloc = (freelistFreeN + freelistPendingN) * tx.db.pageSize | ||
| tx.db.stats.FreelistInuse = freelistAlloc | ||
| tx.db.stats.TxStats.add(&tx.stats) | ||
| tx.db.statlock.Unlock() | ||
| } | ||
| } else { | ||
| tx.db.removeTx(tx) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry to say this, but did you read the comment? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It changes it to a pointer. So I think it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you guys have a raspi or any other arm32 device sitting around to validate this? I'm not so worried about the stats pointer, but rather the
sync.Onceorsync.Poolcould become problematic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just quickly ran the tests on your branch on my pi - so everything works as intended. Sorry I'm in the middle of a move, luckily I had it plugged in and could ssh into it. Not sure whether you have your QEMU setup still somewhere...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qemu-user-staticmight be good enough to verify this. #577 (comment)I think we might need to setup a workflow to guarantee it won't be broken in future.
The known issue https://pkg.go.dev/sync/atomic#pkg-note-BUG seems only specific to
sync.atomicpackage. But I agree that we should verify it anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @ivanvc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx @ivanvc pls refer to #665