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

proposal: time: new Init() and Start() methods for Timer #54870

Closed
OmidHekayati opened this issue Sep 5, 2022 · 13 comments
Closed

proposal: time: new Init() and Start() methods for Timer #54870

OmidHekayati opened this issue Sep 5, 2022 · 13 comments
Labels
Milestone

Comments

@OmidHekayati
Copy link

As suggested here #54861 by add these two new methods to time package, we achieve this goals:

  • Changes don't break any existing code and respect backward compatibility.
  • prevent two heap allocation (unnecessary) request, when a Timer can be part of other structs (embed or as a field).
  • Due to NewTimer can't inline and return the pointer of a Timer struct,
    If devs want to add a Timer to other structs (embed or as a field) to allocate both structs in one request,
    existing NewTimer() don't let us do this.
  • Init method can inline now and the Start() method sets the when field. In this way, functions respect the single responsibility principle.
/time> go build -gcflags="-m"
./sleep.go:64:6: can inline (*Timer).Init
./sleep.go:50:8: inlining call to (*Timer).Init
  • Improve code readability of NewTimer() function.
@gopherbot gopherbot added this to the Proposal milestone Sep 5, 2022
@seankhliao
Copy link
Member

please include the proposed function signatures and a description of what they do here.

@OmidHekayati
Copy link
Author

Proposed function signatures:

func (t *Timer) Init()
func (t *Timer) Start(d Duration)

func (t *Ticker) Init()
func (t *Ticker) Tick(d Duration)

Keep in mind that these new methods is not introduce new functionality, They just get some functionalities from NewTimer & NewTicker and made them be simple. Init() method as usuall initialize the new allocated object. Start() or Tick() method will start timer(runtime package) that will send signal (on the channel) once or on periods base on its type.

@ianlancetaylor
Copy link
Contributor

Please write doc comments for these new methods. Thanks. In particular, what does the Init method do? Why do we need it?

@OmidHekayati
Copy link
Author

Although I wrote comments in #54861, you are right I must copy them here for better referencing.

// Init initialize the Timer.
// It must call before call Start() or Reset() methods
func (t *Timer) Init()
// Start start the timer that will send
// the current time on its channel after at least duration d.
func (t *Timer) Start(d Duration)

// Init initialize the Ticker.
// It must call before call Tick() or Reset() methods
func (t *Ticker) Init()
// Tick start the timer that will send
// the current time on its channel after each tick.
func (t *Ticker) Tick(d Duration)

As said in the first comment, Due to NewTimer or NewTicker returning a pointer of Timer or Ticker and they can't be inlined, So when we can embed Timer or Ticker we don't have a way to Init them without unnecessary heap allocation. When Init exist we can easily add Timer or Ticker to other structs with one heap allocation request.

type HeartBeatService struct {
	// other fields
	interval time.Duration
	ticker   time.Ticker
}

func (h *HeartBeatService) Init(interval time.Duration) {
	h.interval = interval
	h.ticker.Init()
}
func (h *HeartBeatService) Reinit() {
	h.ticker.Stop()
	// can flush h.ticker.C here too.
}
func (h *HeartBeatService) Deinit() {
	h.ticker.Stop()
}

func (h *HeartBeatService) Process() {
	h.ticker.Start(h.interval)

	for now := range h.ticker.C {
		// send signal
	}
}

In this way, each new methods have a single responsibility and complete the life cycle of Timer or Ticker, And prevents congested logic in NewTimer or NewTicker.

@rsc
Copy link
Contributor

rsc commented Sep 28, 2022

What does Init do that Start cannot do instead?

@OmidHekayati
Copy link
Author

@rsc Init exist to show object(Timer) life cycle and respect functions single responsibility. (Personally I like a code that respect these principles).

But anyway you are the ruler, if you don't like Init exported method, just accept the Start exported method. But take in mind that we need init local method any way, because Timer let us call its Reset method in not started timer. So We need to check for not initialized timer in both Start and Reset or change the Reset documents.

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

It sounds like the goal is purely to avoid allocations. But each Timer is kept in a heap stored in the runtime, and we don't want the runtime's copy to be writable by user code, or else the heap invariants can be violated. So I don't think we want the runtime to allow the user to provide the memory for a Timer.

Is there a program or real-world benchmark you can point to that is suffering from these allocations?

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@OmidHekayati
Copy link
Author

It sounds like the goal is purely to avoid allocations. But each Timer is kept in a heap stored in the runtime, and we don't want the runtime's copy to be writable by user code, or else the heap invariants can be violated.

@rsc How runtime's copy can be writable by user code?
Can you explain when all fields of runtimeTimer are privates user can change them directly without any exported methods?

So I don't think we want the runtime to allow the user to provide the memory for a Timer.

When any developer decides to embed a structure into another one, It means that have enough knowledge about bad things that may happen like memory leaks and it is like many other features in Go such as string that can be a huge problem if we don't be careful.

Is there a program or real-world benchmark you can point to that is suffering from these allocations?

I know the usage of proposed APIs is rare (in comparison to other features) and usually used in low-level libraries such as /runtime/netpoll.go/pollDesc but it can be a helpful feature in some use-cases like implementing network protocols in user space (instead of kernel space) as QUIC.
I think writing many things in user space (as OS-library) is the future way to develop computer software.

P.S.: I try to rewrite the timer package and get all functionality away from the Go runtime and time packages, It is not ready to use yet but it is a possible way.
I think the Go runtime package needs many rewrite to be small and just a wrapper of functionalities not an implementor of them.
https://github.com/GeniusesGroup/libgo/tree/dev/timer

@ianlancetaylor
Copy link
Contributor

The fact that the fields are private doesn't prevent someone from writing

    var v HeartBeatService
    v.Init()
    v  = HeartBeatService{}

I try to rewrite the timer package and get all functionality away from the Go runtime and time packages, It is not ready to use yet but it is a possible way.
I think the Go runtime package needs many rewrite to be small and just a wrapper of functionalities not an implementor of them.

That is interesting. The current timer implementation is closely tied to the scheduler, as I expect you know. This has been guided by a desire for efficiency and timer accuracy, especially for programs with millions of timers. That is, performance and accuracy are extremely important here. If we can get comparable results by moving this out of the runtime, that would be great. But it sounds hard.

@OmidHekayati
Copy link
Author

The fact that the fields are private doesn't prevent someone from writing

    var v HeartBeatService
    v.Init()
    v  = HeartBeatService{}

/runtime/time.go/timerNoStatus is the answer that if any developer tries to do in your way, in many logics in timing like this will drop its timer. I know it is a bug but the bug came from the developer's mistake not the timer package that try to reallocation without Init and Start the new Timer

I try to rewrite the timer package and get all functionality away from the Go runtime and time packages, It is not ready to use yet but it is a possible way.
I think the Go runtime package needs many rewrite to be small and just a wrapper of functionalities not an implementor of them.

That is interesting. The current timer implementation is closely tied to the scheduler, as I expect you know. This has been guided by a desire for efficiency and timer accuracy, especially for programs with millions of timers. That is, performance and accuracy are extremely important here. If we can get comparable results by moving this out of the runtime, that would be great. But it sounds hard.

Until now, I know we can't have the timer package without extracting some other functionality from runtime. we need some functionality of the scheduler, a user-space scheduler like Go or kernel level one.

In overall I know it is not so hard to split the Go runtime package, It is just about giving responsibility to some ones that have enough rights.
I suggest starting by taking away all unix time codes to small a package in time/unix. I can do it for the community like this one But it needs very long process decision that make many overheads due to conflicts from other developers.

@rsc
Copy link
Contributor

rsc commented Apr 19, 2023

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented May 3, 2023

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

5 participants