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

HostInfo: reduce mutex.Locks #165

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

moguchev
Copy link

@moguchev moguchev commented Apr 8, 2024

I know that there is comment by @Zariel to reduce locking.

I didn't thoroughly refactor the code, but I found fast and cheap optimization in functions (*HostInfo).HostName and (*HostInfo).HostNameAndPort.

In our case of #164 (*HostInfo).HostName leads to spending a lot of time in Lock...
Also in QueryObserver and BatchObserver we calls public methods of HostInfo (HostName , HostNameAndPort) for metrics and we have a lot of mutex locks.

host_source.go Outdated
@@ -410,16 +410,43 @@ func (h *HostInfo) IsUp() bool {
}

func (h *HostInfo) HostnameAndPort() string {
// Fast path: in 99.9% hostname is not empty
Copy link
Collaborator

Choose a reason for hiding this comment

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

How did you deduce this number?

Copy link
Author

Choose a reason for hiding this comment

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

99.9% is certainly a big number.

We actively use the public methods Hostname() and HostnameAndPort() in metrics.QueryObserver. Accordingly, for each request in scylladb we call Hostname()/HostnameAndPort(), but if the cluster is stable and no reccconections occur, then hostname in most cases is not empty.

I'll attach go profile of mutex (before -> after) later

Copy link
Author

Choose a reason for hiding this comment

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

Before patch:
Снимок экрана 2024-05-17 в 17 00 34

Copy link
Author

Choose a reason for hiding this comment

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

After:
Снимок экрана 2024-05-17 в 17 00 45

host_source.go Outdated
addr, _ := h.connectAddressLocked()
h.hostname = addr.String()
}
return net.JoinHostPort(h.hostname, strconv.Itoa(h.port))
}

func (h *HostInfo) Hostname() string {
// Fast path: in 99.9% hostname is not empty
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as before

host_source.go Outdated Show resolved Hide resolved
@moguchev moguchev force-pushed the host_info_reduce_locks branch 3 times, most recently from c9405f1 to f8f2b91 Compare May 15, 2024 19:29
@roydahan
Copy link
Collaborator

Please add a descriptive commit message to your commit.

@dkropachev
Copy link
Collaborator

I would recommend to use atomic.Pointer instead, please take a look this paper

Since HostInfo experiance mostly read access, referred solution is going to be up to 90 times faster than this one.

@mykaul
Copy link

mykaul commented Jun 10, 2024

@moguchev - can you refresh the PR, addressing above comments?

@moguchev
Copy link
Author

@moguchev - can you refresh the PR, addressing above comments?

Yes, I’ll do it

@dkropachev
Copy link
Collaborator

@moguchev , I have given a try to atomic update, it is too intrusive and is going to create lot's of conflict with upstream, so it is better not to do that

@sylwiaszunejko sylwiaszunejko merged commit dce12c1 into scylladb:master Jun 20, 2024
1 check passed
@mykaul
Copy link

mykaul commented Jun 20, 2024

Thanks @moguchev for your contribution!

@mykaul
Copy link

mykaul commented Jul 8, 2024

1.14.2 was released with your contribution @moguchev - thanks again!

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

Successfully merging this pull request may close these issues.

6 participants