kreeben / resin

Vector space index based search engine that's available as a HTTP service or as an embedded library.
MIT License
568 stars 40 forks source link

GetTicks/GetNextChronologicalFileId question #50

Closed AlgorithmsAreCool closed 6 years ago

AlgorithmsAreCool commented 7 years ago

I was looking over your commits and noticed your changes to GetTicks() in https://github.com/kreeben/resin/commit/139da038f61fcbd71a5695e2d667d4d052e4185f

Since GetNextChronologicalFileId() is the sole consumer of GetTicks() I am trying to understand what you are trying to do with it? Before it was just a wrapper for DateTime.Now.Ticks, but now it is an incrementing number. The implementation of GetNext() is also not theadsafe. Since Random isn't a thread safe type and the Ticks++ is not guaranteed to be atomic. In fact i'm not sure what the use of Random is besides to introduce a delay in the function.

Based on the name of GetNextChronologicalFileId() and the commit message I am assuming it is intended to be:

Are there any other rules that this function must follow?

kreeben commented 7 years ago

Thanks for taking interest in the Resin codebase and sorry for waiting twenty days until a reply.

The current GetTicks and, more importantly, GetNextChronologicalFileId, is here.

The requirements, unfulfilled as you rightly noted, are:

I used Random to create a delay that would not be optimized away, like an empty for-loop might be.

mdissel commented 7 years ago

To prevent timezone daylight saving time you should use datetime.utc.ticks as an initial value.

AlgorithmsAreCool commented 7 years ago

You are barking up the wrong tree a bit here.

This implementation will satisfy your requirements. It is extremely fast, does not require any type of stalling or waiting and it also requires no locks which is a nice to have.

Interlocked is definitely the way to go for this type of thing.

using System.Threading;
...

private long currentId;

public static long GetNextChronologicalFileId()
{
    return Interlocked.Increment(ref currentId);    
}

When the program starts up you probably want to pickup where you left off and set currentId to the last committed Id value.

kreeben commented 7 years ago

Yes, this certainly looks like the best way of keeping a thread-safe counter. Googling "c# atomic counter" led me to the same answer.

Much obliged.

kreeben commented 6 years ago

N/A as of this: https://github.com/kreeben/resin/commit/5f85425a0f61bbfbe2b2676d71d72f37677a0bef (i.e. there is no longer contention to grab a new ID)