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

add atomic duration type #37

Merged
merged 5 commits into from
May 1, 2018
Merged

add atomic duration type #37

merged 5 commits into from
May 1, 2018

Conversation

nathanjordan
Copy link
Contributor

Before opening your pull request, please make sure that you've:

Thanks for your contribution!

@CLAassistant
Copy link

CLAassistant commented Apr 25, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 25, 2018

Codecov Report

Merging #37 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #37   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines         118    132   +14     
=====================================
+ Hits          118    132   +14
Impacted Files Coverage Δ
atomic.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8474b86...3780cbb. Read the comment docs.

atomic.go Outdated
// Duration is an atomic wrapper around time.Duration
// https://godoc.org/time#Duration
type Duration struct {
v *Int64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

would it make more sense to reimplement the underlying int64 bit so we don't have a double-pointer situation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

don't need to reimplement, you can just make this v Int64

Copy link
Collaborator

Choose a reason for hiding this comment

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

Use Int64 instead of a pointer

atomic.go Outdated
// Duration is an atomic wrapper around time.Duration
// https://godoc.org/time#Duration
type Duration struct {
v *Int64
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't need to reimplement, you can just make this v Int64

atomic.go Outdated
func (d *Duration) Store(s time.Duration) {
d.v.Store(int64(s))
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

after looking at this, i think it might make sense to add Add, Sub, CAS, similar to Int64, but are atomic

@nathanjordan
Copy link
Contributor Author

I benchmarked reusing the existing Int64 type and using a raw int64 and there was a negligible performance impact between them so I'm going to just reuse the existing type.

# int64
$ go test -bench BenchmarkStress/duration ./
goos: darwin
goarch: amd64
pkg: go.uber.org/atomic
BenchmarkStress/duration/serial-8  	30000000	        43.5 ns/op
BenchmarkStress/duration/parallel-8         	10000000	       155 ns/op
PASS
ok  	go.uber.org/atomic	3.095s

# uber-go/atomic.Int64
$ go test -bench BenchmarkStress/duration ./
goos: darwin
goarch: amd64
pkg: go.uber.org/atomic
BenchmarkStress/duration/serial-8  	30000000	        42.7 ns/op
BenchmarkStress/duration/parallel-8         	10000000	       158 ns/op

atomic.go Outdated
// Duration is an atomic wrapper around time.Duration
// https://godoc.org/time#Duration
type Duration struct {
v *Int64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use Int64 instead of a pointer

@nathanjordan nathanjordan merged commit 1ea20fb into master May 1, 2018
@prashantv prashantv deleted the nathan/duration branch May 1, 2019 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants