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.

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

fix data race on tx.Stats #373

Merged
merged 7 commits into from
Jan 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
81 changes: 76 additions & 5 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
@@ -1,18 +1,89 @@
name: Tests
on: [push, pull_request]
jobs:
test:
test-linux:
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, windows-latest]
runs-on: ${{ matrix.os }}
target:
- linux-amd64-unit-test-4-cpu
Copy link
Member

Choose a reason for hiding this comment

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

It would be better we have the following three targets to cover the no-race cases:

  • linux-amd64-unit-test-1-cpu
  • linux-amd64-unit-test-2-cpu
  • linux-amd64-unit-test-4-cpu

Of course, it can be addressed in a separate PR if there is no any objection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do!

- linux-amd64-unit-test-4-cpu-freelist-hashmap-race
- linux-amd64-unit-test-2-cpu-freelist-array-short-race
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-go@v3
with:
go-version: "1.17.13"
- run: make fmt
- run: make race
- run: make test
- env:
TARGET: ${{ matrix.target }}
run: |
case "${TARGET}" in
linux-amd64-unit-test-4-cpu)
CPU=4 make test
;;
linux-amd64-unit-test-4-cpu-freelist-hashmap-race)
CPU=4 ENABLE_RACE=true make test-freelist-hashmap
;;
linux-amd64-unit-test-2-cpu-freelist-array-short-race)
CPU=2 ENABLE_RACE=true EXTRA_TESTFLAGS="-short" make test-freelist-array
;;
*)
echo "Failed to find target"
exit 1
;;
esac
- name: golangci-lint
uses: golangci/golangci-lint-action@0ad9a0988b3973e851ab0a07adf248ec2e100376 # v3.3.1

test-windows:
strategy:
fail-fast: false
matrix:
target:
- windows-amd64-unit-test-4-cpu
# FIXME(fuweid):
#
# The windows will throws the following error when enable race.
# We skip it until we have solution.
#
# ThreadSanitizer failed to allocate 0x000200000000 (8589934592) bytes at 0x0400c0000000 (error code: 1455)
#
#- windows-amd64-unit-test-4-cpu-race
runs-on: windows-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-go@v3
with:
go-version: "1.17.13"
- run: make fmt
- env:
TARGET: ${{ matrix.target }}
run: |
case "${TARGET}" in
windows-amd64-unit-test-4-cpu)
CPU=4 make test
;;
*)
echo "Failed to find target"
exit 1
;;
esac
shell: bash
- name: golangci-lint
uses: golangci/golangci-lint-action@0ad9a0988b3973e851ab0a07adf248ec2e100376 # v3.3.1

coverage:
needs: ["test-linux", "test-windows"]
strategy:
matrix:
os: [ubuntu-latest, windows-latest]
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v3
- uses: actions/setup-go@v3
with:
go-version: "1.17.13"
- run: make coverage

1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
*.swp
/bin/
cover.out
cover-*.out
/.idea
*.iml
38 changes: 28 additions & 10 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,42 @@ BRANCH=`git rev-parse --abbrev-ref HEAD`
COMMIT=`git rev-parse --short HEAD`
GOLDFLAGS="-X main.branch $(BRANCH) -X main.commit $(COMMIT)"

race:
@TEST_FREELIST_TYPE=hashmap go test -v -race -test.run="TestSimulate_(100op|1000op)"
@echo "array freelist test"
@TEST_FREELIST_TYPE=array go test -v -race -test.run="TestSimulate_(100op|1000op)"
TESTFLAGS_RACE=-race=false
ifdef ENABLE_RACE
TESTFLAGS_RACE=-race=true
endif

TESTFLAGS_CPU=
ifdef CPU
TESTFLAGS_CPU=-cpu=$(CPU)
endif
TESTFLAGS = $(TESTFLAGS_RACE) $(TESTFLAGS_CPU) $(EXTRA_TESTFLAGS)

fmt:
!(gofmt -l -s -d $(shell find . -name \*.go) | grep '[a-z]')

lint:
golangci-lint run ./...

test:
TEST_FREELIST_TYPE=hashmap go test -timeout 30m -v -coverprofile cover.out -covermode atomic
TEST_FREELIST_TYPE=hashmap go test -v ./cmd/bbolt
test: test-freelist-hashmap test-freelist-array

test-freelist-hashmap:
@echo "hashmap freelist test"
TEST_FREELIST_TYPE=hashmap go test -v ${TESTFLAGS} -timeout 30m
TEST_FREELIST_TYPE=hashmap go test -v ${TESTFLAGS} ./cmd/bbolt

test-freelist-array:
@echo "array freelist test"
TEST_FREELIST_TYPE=array go test -v ${TESTFLAGS} -timeout 30m
TEST_FREELIST_TYPE=array go test -v ${TESTFLAGS} ./cmd/bbolt

@TEST_FREELIST_TYPE=array go test -timeout 30m -v -coverprofile cover.out -covermode atomic
@TEST_FREELIST_TYPE=array go test -v ./cmd/bbolt
coverage:
@echo "hashmap freelist test"
TEST_FREELIST_TYPE=hashmap go test -v -timeout 30m \
-coverprofile cover-freelist-hashmap.out -covermode atomic

@echo "array freelist test"
TEST_FREELIST_TYPE=array go test -v -timeout 30m \
-coverprofile cover-freelist-array.out -covermode atomic

.PHONY: race fmt test lint
.PHONY: fmt test test-freelist-hashmap test-freelist-array lint
Copy link
Contributor

Choose a reason for hiding this comment

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

Now it's still redundant... test resolves to both: test-freelist-{...} targets. And no race run by default at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this combination in action and 75% successful rate for the race matrix 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me add the following matrix in the follow-up pr.

     linux-amd64-unit-test-2-cpu-simulate-race)
           CPU=2 ENABLE_RACE=true EXTRA_TESTFLAGS="-test.run='TestSimulate_(100op|1000op)'" make test            
           ;;

8 changes: 5 additions & 3 deletions allocate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,16 @@ func TestTx_allocatePageStats(t *testing.T) {
pages: make(map[pgid]*page),
}

prePageCnt := tx.Stats().PageCount
txStats := tx.Stats()
prePageCnt := txStats.GetPageCount()
allocateCnt := f.free_count()

if _, err := tx.allocate(allocateCnt); err != nil {
t.Fatal(err)
}

if tx.Stats().PageCount != prePageCnt+allocateCnt {
t.Errorf("Allocated %d but got %d page in stats", allocateCnt, tx.Stats().PageCount)
txStats = tx.Stats()
if txStats.GetPageCount() != prePageCnt+int64(allocateCnt) {
t.Errorf("Allocated %d but got %d page in stats", allocateCnt, txStats.GetPageCount())
}
}
4 changes: 2 additions & 2 deletions bucket.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (b *Bucket) Writable() bool {
// Do not use a cursor after the transaction is closed.
func (b *Bucket) Cursor() *Cursor {
// Update transaction statistics.
b.tx.stats.CursorCount++
b.tx.stats.IncCursorCount(1)

// Allocate and return a cursor.
return &Cursor{
Expand Down Expand Up @@ -681,7 +681,7 @@ func (b *Bucket) node(pgid pgid, parent *node) *node {
b.nodes[pgid] = n

// Update statistics.
b.tx.stats.NodeCount++
b.tx.stats.IncNodeCount(1)

return n
}
Expand Down
12 changes: 7 additions & 5 deletions db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,9 @@ func TestDB_Concurrent_WriteTo(t *testing.T) {
panic(err)
}
f.Close()
snap := btesting.MustOpenDBWithOption(t, f.Name(), o)

copyOpt := *o
snap := btesting.MustOpenDBWithOption(t, f.Name(), &copyOpt)
defer snap.MustClose()
snap.MustCheck()
}
Expand Down Expand Up @@ -1036,8 +1038,8 @@ func TestDB_Stats(t *testing.T) {
}

stats := db.Stats()
if stats.TxStats.PageCount != 2 {
t.Fatalf("unexpected TxStats.PageCount: %d", stats.TxStats.PageCount)
if stats.TxStats.GetPageCount() != 2 {
t.Fatalf("unexpected TxStats.PageCount: %d", stats.TxStats.GetPageCount())
} else if stats.FreePageN != 0 {
t.Fatalf("unexpected FreePageN != 0: %d", stats.FreePageN)
} else if stats.PendingPageN != 2 {
Expand Down Expand Up @@ -1120,8 +1122,8 @@ func TestDBStats_Sub(t *testing.T) {
b.TxStats.PageCount = 10
b.FreePageN = 14
diff := b.Sub(&a)
if diff.TxStats.PageCount != 7 {
t.Fatalf("unexpected TxStats.PageCount: %d", diff.TxStats.PageCount)
if diff.TxStats.GetPageCount() != 7 {
t.Fatalf("unexpected TxStats.PageCount: %d", diff.TxStats.GetPageCount())
}

// free page stats are copied from the receiver and not subtracted
Expand Down
12 changes: 6 additions & 6 deletions internal/btesting/btesting.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,14 +189,14 @@ func (db *DB) CopyTempFile() {
func (db *DB) PrintStats() {
var stats = db.Stats()
fmt.Printf("[db] %-20s %-20s %-20s\n",
fmt.Sprintf("pg(%d/%d)", stats.TxStats.PageCount, stats.TxStats.PageAlloc),
fmt.Sprintf("cur(%d)", stats.TxStats.CursorCount),
fmt.Sprintf("node(%d/%d)", stats.TxStats.NodeCount, stats.TxStats.NodeDeref),
fmt.Sprintf("pg(%d/%d)", stats.TxStats.GetPageCount(), stats.TxStats.GetPageAlloc()),
fmt.Sprintf("cur(%d)", stats.TxStats.GetCursorCount()),
fmt.Sprintf("node(%d/%d)", stats.TxStats.GetNodeCount(), stats.TxStats.GetNodeDeref()),
)
fmt.Printf(" %-20s %-20s %-20s\n",
fmt.Sprintf("rebal(%d/%v)", stats.TxStats.Rebalance, truncDuration(stats.TxStats.RebalanceTime)),
fmt.Sprintf("spill(%d/%v)", stats.TxStats.Spill, truncDuration(stats.TxStats.SpillTime)),
fmt.Sprintf("w(%d/%v)", stats.TxStats.Write, truncDuration(stats.TxStats.WriteTime)),
fmt.Sprintf("rebal(%d/%v)", stats.TxStats.GetRebalance(), truncDuration(stats.TxStats.GetRebalanceTime())),
fmt.Sprintf("spill(%d/%v)", stats.TxStats.GetSpill(), truncDuration(stats.TxStats.GetSpillTime())),
fmt.Sprintf("w(%d/%v)", stats.TxStats.GetWrite(), truncDuration(stats.TxStats.GetWriteTime())),
)
}

Expand Down
8 changes: 4 additions & 4 deletions node.go
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ func (n *node) splitTwo(pageSize uintptr) (*node, *node) {
n.inodes = n.inodes[:splitIndex]

// Update the statistics.
n.bucket.tx.stats.Split++
n.bucket.tx.stats.IncSplit(1)

return n, next
}
Expand Down Expand Up @@ -391,7 +391,7 @@ func (n *node) spill() error {
}

// Update the statistics.
tx.stats.Spill++
tx.stats.IncSpill(1)
}

// If the root node split and created a new root then we need to spill that
Expand All @@ -413,7 +413,7 @@ func (n *node) rebalance() {
n.unbalanced = false

// Update statistics.
n.bucket.tx.stats.Rebalance++
n.bucket.tx.stats.IncRebalance(1)

// Ignore if node is above threshold (25%) and has enough keys.
var threshold = n.bucket.tx.db.pageSize / 4
Expand Down Expand Up @@ -547,7 +547,7 @@ func (n *node) dereference() {
}

// Update statistics.
n.bucket.tx.stats.NodeDeref++
n.bucket.tx.stats.IncNodeDeref(1)
}

// free adds the node's underlying page to the freelist.
Expand Down
Loading