search

opendlang / opend

Boost Software License 1.0
76 stars 17 forks source link

Adds core.sync.event.EventAwaiter #64

Open denizzzka opened 3 months ago

denizzzka commented 3 months ago

Checking whether the Event object is in initialized state every time then any of its methods are called is waste of CPU resources

I propose to intruduce new event struct which provides RAII interface and methods doesn't poisoned by if (m_initalized) checks

IgorDeepakM commented 3 months ago

Is there a reason why there should be a completely new file event2.d for this? Why not append the additional type to event.d. You are allowed to have two types inside one file.

Also the name Event2 is kind of ambiguous. People will ask, why are there two versions of Event? A better name is EventS or EventStruct so that it is more obvious what it is about.

adamdruppe commented 3 months ago

Yeah, and they're both structs too...

IgorDeepakM commented 3 months ago

Yeah, and they're both structs too...

Just too used that everything are classes outwards. Then another name like EventNoInitCheck or something.

denizzzka commented 3 months ago

@IgorDeepakM

Is there a reason why there should be a completely new file event2.d for this?

No

Why not append the additional type to event.d. You are allowed to have two types inside one file.

Yes, I can change this without any problems

Also the name Event2 is kind of ambiguous. People will ask, why are there two versions of Event? A better name is EventS or EventStruct so that it is more obvious what it is about.

Agree

@adamdruppe Which name do you like best?

@adamdruppe

Yeah, and they're both structs too...

Do you mean that they should be a classes? To avoid copying, makes sense But I thought that such changes should not be made because events are involved in garbage collection, and I think it may cause some kind of endless loop of recursion

denizzzka commented 3 months ago

Yeah, and they're both structs too...

Just too used that everything are classes outwards. Then another name like EventNoInitCheck or something.

EventAwaiter?

IgorDeepakM commented 3 months ago

Another question is if we really need the old one. Why wouldn't it be initialized? That would be if the OS primitives fails to be created but one can argue if that is a fatal error.

For the other primitives like mutex and semaphores, there are no such checks and if they fail there is usually an assert or Error is being thrown.

If there is a chicken and egg problem during druntime initialization, then i think such special cases should be handled outside the Event type. At most there could be a isInitialized() method that checks if it is initialized.

adamdruppe commented 3 months ago

I think the original one is how it is because D doesn't have any kind of default constructor, so you might forget to initialize it. Maybe just @disable this(); on that would be ok to make it work.

denizzzka commented 3 months ago

Another question is if we really need the old one.

For backward compatibility with D: old Event isn't fails if struct isn't initialized and such behaviour could been used somehere

IgorDeepakM commented 3 months ago

Another question is if we really need the old one.

For backward compatibility with D: old Event isn't fails if struct isn't initialized and such behaviour could been used somehere

I'd say we just brute force this one. Just replace the old one with the new one. Remove setIfInitialized() and replace it with just set(). setIfInitialized() can just call set() if we run into problems.

adamdruppe commented 3 months ago

setIfInitialized is a new thing anyway. It was called set until... some time last year. Very recent.

denizzzka commented 3 months ago

setIfInitialized is a new thing anyway. It was called set until... some time last year. Very recent.

Yes, I renamed it: https://github.com/dlang/dmd/pull/15800

IgorDeepakM commented 3 months ago

I actually meant remove Event and replace it with EventAwaiter (named just Event). Then the code inside the opend repo that uses setIfInitialized needs to be changed to set.

denizzzka commented 3 months ago

I actually meant remove Event and replace it with EventAwaiter (named just Event).

This can lead to hard to find errors in a code that relies on possibility of not initializing an Event

Then the code inside the opend repo that uses setIfInitialized needs to be changed to set.

Here is no such code except already changed in this PR

upd: Besides this, I think the original name is incorrect: this structure itself is not an "event". This PR is a good reason to fix name too.