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

hlc: remove locking in Physical(Now|Time), addresses 30520 #32225

Merged
merged 1 commit into from Nov 12, 2018

Conversation

Projects
None yet
6 participants
@ajwerner
Copy link
Collaborator

ajwerner commented Nov 12, 2018

Fixes #30520

Release note (performance improvement): Remove locking when reading physical time

@ajwerner ajwerner requested a review from nvanbenschoten Nov 12, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 12, 2018

This change is Reviewable

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Nov 12, 2018

CLA assistant check
All committers have signed the CLA.

@ajwerner ajwerner added the first-pr label Nov 12, 2018

@nvanbenschoten
Copy link
Member

nvanbenschoten left a comment

Addresses #30520

I don't think Github will pick this up. Try one of these: https://help.github.com/articles/closing-issues-using-keywords/.

@bdarnell, want to take a look?

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/util/hlc/hlc.go, line 222 at r1 (raw file):

}

// getPhysicalClock locks mu in order to access the physical clock, check for

Let's name these methods something even more clear. How about getPhysicalClockAndCheck{Locked}.

@ajwerner ajwerner force-pushed the ajwerner:ajwerner/locking-in-hlc-physical-now branch from b0cb42b to db93ef4 Nov 12, 2018

hlc: remove locking in Physical(Now|Time), addresses 30520
Addresses #30520

Release note (performance improvement): Remove locking when reading physical time

@ajwerner ajwerner force-pushed the ajwerner:ajwerner/locking-in-hlc-physical-now branch from db93ef4 to dfa2f41 Nov 12, 2018

@ajwerner
Copy link
Collaborator

ajwerner left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/util/hlc/hlc.go, line 222 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's name these methods something even more clear. How about getPhysicalClockAndCheck{Locked}.

Done.

@bdarnell
Copy link
Member

bdarnell left a comment

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@nvanbenschoten
Copy link
Member

nvanbenschoten left a comment

:lgtm: nice!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@ajwerner

This comment has been minimized.

Copy link
Collaborator

ajwerner commented Nov 12, 2018

bors r+

@craig

This comment has been minimized.

Copy link

craig bot commented Nov 12, 2018

🔒 Permission denied

Existing reviewers: click here to make ajwerner a reviewer

@nvanbenschoten

This comment has been minimized.

Copy link
Member

nvanbenschoten commented Nov 12, 2018

Just added you as a reviewer. Let's try that again.

@ajwerner

This comment has been minimized.

Copy link
Collaborator

ajwerner commented Nov 12, 2018

bors r+

@craig

This comment has been minimized.

Copy link

craig bot commented Nov 12, 2018

🔒 Permission denied

Existing reviewers: click here to make ajwerner a reviewer

@ajwerner

This comment has been minimized.

Copy link
Collaborator

ajwerner commented Nov 12, 2018

bors r+

@craig

This comment has been minimized.

Copy link

craig bot commented Nov 12, 2018

🔒 Permission denied

Existing reviewers: click here to make ajwerner a reviewer

@nvanbenschoten

This comment has been minimized.

Copy link
Member

nvanbenschoten commented Nov 12, 2018

Ok, try now.

@ajwerner

This comment has been minimized.

Copy link
Collaborator

ajwerner commented Nov 12, 2018

bors r+

craig bot pushed a commit that referenced this pull request Nov 12, 2018

Merge #32225
32225: hlc: remove locking in Physical(Now|Time), addresses 30520 r=ajwerner a=ajwerner

Fixes #30520

Release note (performance improvement): Remove locking when reading physical time

Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 12, 2018

Build succeeded

@craig craig bot merged commit dfa2f41 into cockroachdb:master Nov 12, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@jordanlewis

This comment has been minimized.

Copy link
Member

jordanlewis commented Nov 12, 2018

Sweet! Does this have a noticeable performance impact on any of our tested workloads?

@nvanbenschoten

This comment has been minimized.

Copy link
Member

nvanbenschoten commented Nov 12, 2018

We do see these two methods show up on large machines in CPU and mutex profiles. I've also seen them be the cause of goroutine preemption in execution traces. I'd spin up a single-node cluster with a 32-core machine and hit it with kv95. Take a look at the baseline throughput and those two profile types before and after this change. Let me know if you need help setting that all up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment