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

bench: aggregate adding completed ops for reads #721

Merged
merged 1 commit into from
Apr 21, 2024

Conversation

ivanvc
Copy link
Member

@ivanvc ivanvc commented Apr 12, 2024

Currently, the completed operations are added to the read benchmarks one by one, and given that each operation is atomic, it impacts the benchmark's performance. Change to update only once per cycle, with the total number of reads.

Related to #720

@@ -1380,10 +1380,10 @@ func (cmd *benchCommand) runReadsSequential(db *bolt.DB, options *BenchOptions,

for {
numReads := int64(0)
defer func() { results.AddCompletedOps(numReads) }()
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 debated using a defer or manually adding the call at the end of the loop. I ended up doing it with the defer, as there are some cases in which it would need to be called from inside the if condition if the run had an error and outside, too. So, it feels cleaner. However, I'm unsure if too many defers (one per loop) would create issues with the call stack if there are too many iterations.

Copy link
Member

@ahrtr ahrtr Apr 16, 2024

Choose a reason for hiding this comment

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

Adding defer inside a for loop isn't a good practice.

How about this?

	err := func() error {
		defer func() { results.AddCompletedOps(numReads) }()

		c := tx.Bucket(benchBucketName).Cursor()
		for k, v := c.First(); k != nil; k, v = c.Next() {
			numReads++
			if v == nil {
				return ErrInvalidValue
			}
		}

		return nil
	}()
	if err != nil {
		return err
	}

Copy link
Member

Choose a reason for hiding this comment

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

Note that deferred function won't be called until the surrounding function returns. It means the initial motivation of progress notification is broken. My proposed change above can fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ahrtr, I applied the suggestion. PTAL :)

@ahrtr
Copy link
Member

ahrtr commented Apr 15, 2024

@fuweid @tjungblu PTAL, thanks

@tjungblu
Copy link
Contributor

/lgtm

@ahrtr
Copy link
Member

ahrtr commented Apr 16, 2024

Also we can also do similar enhancement in runWritexxxx. Please also compare the write performance after you finish enhancing the runWritexxx. Thanks

@ivanvc
Copy link
Member Author

ivanvc commented Apr 16, 2024

Also we can also do similar enhancement in runWritexxxx. Please also compare the write performance after you finish enhancing the runWritexxx. Thanks

Hey @ahrtr, unfortunately (as I mentioned in #720 (comment)), changing the runWrite functions doesn't show an improvement. The results don't look consistent (I think it's similar to what @fuweid reported).

@ahrtr
Copy link
Member

ahrtr commented Apr 19, 2024

@ivanvc Please resolve the comment, also read #720 (comment)

@ivanvc
Copy link
Member Author

ivanvc commented Apr 19, 2024

@ivanvc Please resolve the comment, also read #720 (comment)

Sorry, @ahrtr, for some reason, I missed the notification and didn't read your comment. I'll address it later today.

Currently, the completed operations are added to the read benchmarks
one by one, and given that each operation is atomic, it impacts the
benchmark's performance. Change to update only once per cycle, with
the total number of reads.

Signed-off-by: Ivan Valdes <[email protected]>
@ivanvc ivanvc force-pushed the fix-read-benchmark-metrics-overhead branch from 0e36040 to 43c669d Compare April 19, 2024 22:32
@ivanvc ivanvc requested a review from ahrtr April 19, 2024 22:38
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

lgtm

Thanks @ivanvc

cc @fuweid @tjungblu @Elbehery PTAL

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@ahrtr ahrtr merged commit ed61ba6 into etcd-io:main Apr 21, 2024
16 checks passed
@ivanvc ivanvc deleted the fix-read-benchmark-metrics-overhead branch April 21, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants