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.

Sign up for GitHub

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

Jump to bottom

runtime: timer.seq field seems unnecessary #46457

Closed
someblue opened this issue May 30, 2021 · 2 comments
Closed

runtime: timer.seq field seems unnecessary #46457

someblue opened this issue May 30, 2021 · 2 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@someblue
Copy link

What's the purpose of seq field in runtimeTimer?

According to runtime/time.go implementation, runtime/time.go define timer struct, including seq field. This seq field will not be modified by runtime and always keep the value when timer struct was created. The only place to use seq field is the callback of timer.f in runOneTimer.

However, in time/sleep.go, this seq field isn't set when initializing

And the callback runtimeTImer.f doesn't use this field too:

At first I guess this is for some compatible reason. But even when go1.0.1 this seq field is not used: https://github.com/golang/go/blob/go1.0.1/src/pkg/time/sleep.go

The comment each time calling f(arg, now) in the timer goroutine in https://github.com/golang/go/blob/go1.16.4/src/runtime/time.go#L24 is inconsistent with the behavior.

It seems to me the seq field should be removed to help understand source code of timer more easily without confusion, and maybe improve the performance (very very slightly).

Should this seq field be remove? Please let me known if I miss something other!
@seankhliao seankhliao changed the title timer: runtimeTimer seq field seem unnecessary runtime: timer.seq field seems unnecessary May 30, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 30, 2021
@ZekeLu
Copy link
Contributor

ZekeLu commented May 31, 2021

dvyukov 2014/08/30 12:27:38

On 2014/08/29 18:53:11, rsc wrote: > This seems unrelated. Remove.

This is required for Go netpoll. Previously I stored PollDesc* into arg.data, and seq into arg.type. Now *pollDesc occupies whole arg.

see https://codereview.appspot.com/132910043/diff/260001/src/pkg/runtime/time.go#newcode25

@ianlancetaylor
Copy link
Member

We don't use the issue tracker for discussions, only for actionable bug reports. For questions and discussions, see https://golang.org/wiki/Questions.

As the above referenced comment points out, the seq field is used in runtime/netpoll.go, when runOneTimer passes it back.

@golang golang locked and limited conversation to collaborators May 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ZekeLu @ianlancetaylor @someblue @gopherbot @seankhliao