hibernate-reactive icon indicating copy to clipboard operation
hibernate-reactive copied to clipboard

Lock-free BlockingIdentifierGenerator LoHi

Open franz1981 opened this issue 2 years ago • 5 comments

This is a version that pay a single allocation cost per each blockSize to save any synchronized to happen. It can be made gc-free by playing some bit tricks packing both lo/hi into a long (and assuming there are no negative values for both, likely) - or with some more complex coordination mechanism (yet to be designed).

I don't know few things yet before un-drafting:

  1. If many concurrent state.next(hi) happen do we need to enforce validation of (hi,lo) based on existing values? new hi > old hi (or diff by blockSize?) && old low >= blockSize.
  2. If many concurrent state.next(hi) happen do we need to ensure some ordering of updates? eg concurrent next(10), next(20),next(30) updates in this pr are not yet guaranteed to be set in any specific order and I cannot see any ordering enforced in the original code, but better be safe then sorry with concurrency
  3. blockSize is a constant? I'm assuming it is...

franz1981 avatar Apr 24 '23 05:04 franz1981

@Sanne @DavideD I'm a bit worried about the original code:

see

		final long next = next();
		if ( next != -1 ) {
			return CompletionStages.completedFuture( next );
		}
                if ( getBlockSize() <= 1 ) {
			return nextHiValue( connectionSupplier )
					.thenApply( i -> next( i ) );
		}

What happen if 2 concurrent threads both see -1 because they both exhausted the (same) current sequences? They both compete to query the database to acquire new ones and compete (again) to update hi. I think this won't end up well unless there's some missing assumption I don't know about the way things are executed here...

And the same can happen with a combiner as well: while within the combiner you can have more then one updates in flights, each one competing to both hit the DB and refresh the new hi.

franz1981 avatar Apr 24 '23 06:04 franz1981

I'd challenge you in ever noticing the current synchronized on profiling data :)

Sanne avatar Apr 24 '23 07:04 Sanne

I'd challenge you in ever noticing the current synchronized on profiling data :)

eheh I know, but it won't cost anything to make it lock free like this I think, I'm more concerned instead by this -> https://github.com/hibernate/hibernate-reactive/pull/1610#issuecomment-1519467511

franz1981 avatar Apr 24 '23 08:04 franz1981

To quote myself of the future (and @Sanne ) re https://github.com/hibernate/hibernate-reactive/pull/1610#issuecomment-1519467511

this is not a "real" problem leading to break anything, but just an efficiency one; in short, if more then one concurrently try to increase the sequence due to exhaustion and succeed, but update hi/lo in the "wrong" order (ie by creating gaps or "jumping forward" before previously existing ones will be "consumed") it can just cause the consumers to request to the DB more sequences then necessary (again) because in both cases (missing new or "old" created batches) would still cause someone to be left unsatisfied. The existing tests are not verifying this hence there was no reason to noticed that.

franz1981 avatar Apr 24 '23 10:04 franz1981

@Sanne undrafting, but feel free to pick what you like for next works on this bud ;)

franz1981 avatar Apr 24 '23 10:04 franz1981