Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

test: add simple arc cache benchmarks #65

Closed
wants to merge 8 commits into from
Closed

Conversation

frrist
Copy link
Member

@frrist frrist commented Apr 6, 2021

Based on v1.0.0

What

Setup for work for completing #64. Adds simple benchmarks for putting and getting blocks from the arc cache.

TODO

  • Expand benchmarks for more specific cases
    • Parallel Operations: 16 and 500 threads per issue description.

@frrist frrist self-assigned this Apr 6, 2021
@welcome
Copy link

welcome bot commented Apr 6, 2021

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines

    if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

case <-t.ctx.Done():
return
default:
idx := rand.Intn(t.numBlocks - 1)
Copy link

Choose a reason for hiding this comment

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

Suggestion: give each goroutine it's own rand.Source. The package level functions like rand.Intn use a shared source with a mutex.

@BigLep
Copy link

BigLep commented Apr 24, 2021

@frrist : this was from your tribute week, right? Should this get published (non draft) so it can then be merged?

@@ -259,3 +352,187 @@ func TestPutManyCaches(t *testing.T) {
trap("PunMany has hit datastore", cd, t)
Copy link
Member

Choose a reason for hiding this comment

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

I know it's not actually in the diff, but I happen to see a typo here.

(And it's the best typo.)

@warpfork
Copy link
Member

I'm a little worried with the way the thrash tests interact with the golang benchmark system.

Example output (from #66) is:

Benchmark_ThrashPut/1_threads-1000000_blocks-8         	     582	   7650691 ns/op	    4534 B/op	      86 allocs/op
Benchmark_ThrashPut/32_threads-1000000_blocks-8        	      61	 154914035 ns/op	 1332696 B/op	   27433 allocs/op
Benchmark_ThrashPut/64_threads-1000000_blocks-8        	      13	 242771029 ns/op	 4198722 B/op	   84806 allocs/op
Benchmark_ThrashPut/500_threads-1000000_blocks-8       	       1	2589767750 ns/op	289443720 B/op	 6513744 allocs/op
Benchmark_ThrashGet/1_threads-1000000_blocks-8         	     538	   2073241 ns/op	    1992 B/op	      38 allocs/op
Benchmark_ThrashGet/32_threads-1000000_blocks-8        	     536	   2088633 ns/op	   18217 B/op	     363 allocs/op
Benchmark_ThrashGet/64_threads-1000000_blocks-8        	     531	   2066803 ns/op	   34843 B/op	     690 allocs/op
Benchmark_ThrashGet/500_threads-1000000_blocks-8       	     710	   1812926 ns/op	  206679 B/op	    4920 allocs/op
Benchmark_ThrashDelete/1_threads-1000000_blocks-8      	     387	   2762988 ns/op	    1786 B/op	      34 allocs/op
Benchmark_ThrashDelete/32_threads-1000000_blocks-8     	       8	 140120505 ns/op	 1252785 B/op	   24420 allocs/op
Benchmark_ThrashDelete/64_threads-1000000_blocks-8     	      14	 238283676 ns/op	 4298582 B/op	   87490 allocs/op
Benchmark_ThrashDelete/500_threads-1000000_blocks-8    	       1	2642262542 ns/op	283855312 B/op	 6381422 allocs/op

The trouble here is A) (minor) it's hard to read without doing additional math on the side, and B) (potentially major) the amount of time taken per "op" is causing the benchmarking system to scale down the run count all the way to one, which means the result could be quite random.

Also, big plus-one to iand's comment about shared sources of randomness introducing other mutexing which could be throwing off results in interesting ways.

@frrist
Copy link
Member Author

frrist commented Apr 26, 2021

@BigLep - yes, this was from my tribute week. I was waiting for some feedback to land before moving this out of draft. Will publish now. I've also reran benchmarks and updated descriptions for PRs: #66 #67 #68 & #69

@frrist frrist marked this pull request as ready for review April 26, 2021 19:40
@mvdan
Copy link
Contributor

mvdan commented Apr 28, 2021

FYI @frrist I'm continuing on this during my tribute week, so I'll work on top of your bench branch in a separate mvdan/arc-cache-bench branch. I'll post updates on #64 as I have them. I'm letting you know so we don't duplicate work or run into conflicts with each other :)

@frrist
Copy link
Member Author

frrist commented May 4, 2021

closing in favor of #70

@frrist frrist closed this May 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants