proposed fix for #707 #708

gavinking · 2021-04-15T10:50:39Z

Make sure that queued requests use the right session and entity args.

gavinking · 2021-04-15T10:51:26Z

OK, @Sanne, @DavideD, @stuartwdouglas, WDYT of this?

Obviously I have not tested it.

Sanne · 2021-04-15T10:59:19Z

I think we need good tests for this area, not least to check how this behaves when the consumer rate is higher than our ability to produce new ids.

make sure that queued requests use the right session and entity args

gavinking · 2021-04-15T11:14:21Z

I think we need good tests for this area

Well, yeah, clearly. The problem is that we have precisely no tests for concurrent operation in the HR project, and I'm not clear even on the right way to write them.

gavinking · 2021-04-15T11:15:17Z

(I force-pushed to remove the useless Queued class and just use a Runnable instead.)

stuartwdouglas · 2021-04-15T11:17:18Z

Looks good, but if we have n consumers waiting maybe we should make sure we request at least n IDs? At the moment by default it is one at a time, so this is basically a global lock in terms of persist performance.

gavinking · 2021-04-15T11:25:01Z

Looks good, but if we have n consumers waiting maybe we should make sure we request at least n IDs? At the moment by default it is one at a time, so this is basically a global lock in terms of persist performance.

Well the assumption I made was that you always choose a big enough block size that you "rarely" need to request a new block. If that's true, then I think having more requests waiting than the size of the block is an extremely low-probability event.

On the other hand, what I realized earlier was that, ooops, what about people who don't want blocks, and set the block size to 1? We should actually be bypassing all this logic entirely in that case, and we don't. Not quite sure how I overlooked that.

gavinking · 2021-04-15T11:30:09Z

if we have n consumers waiting maybe we should make sure we request at least n IDs?

Also, at the time we request an hi value, we have zero consumers waiting.

gavinking · 2021-04-15T11:34:20Z

Not quite sure how I overlooked that.

Well, hrm, actually I supposed it's not entirely clear what the best behavior here is.

For sequences, yes, I think the right thing to do is not wait for concurrent requests to complete.

For table generation it's not so clear, however.

related to discussion around hibernate#707 This seems right for sequences. I'm not sure about tables though.

gavinking · 2021-04-15T11:37:34Z

...-reactive-core/src/main/java/org/hibernate/reactive/id/impl/BlockingIdentifierGenerator.java

+            //special case where we're not using blocking at all
+            return nextHiValue(session);
+        }
+


@stuartwdouglas WDYT?

Sanne · 2021-04-15T11:59:22Z

FWIW I had written a POC about a year ago to have such an optimizer, which would tune the page sized in an "adaptative" way.

I abandoned it as I needed to learn how to test such things with vertx and never had much time - but it sounds like conceptually you might want to revive the idea.

gavinking · 2021-04-15T12:14:03Z

I mean in principle it would not be at all hard to increase the block size if we find ourselves requesting too many new blocks. Not hard at all.

Sanne · 2021-04-15T12:39:35Z

Right - and there's several other smart things one can easily do in the reactive model, such as we could already trigger a block allocation before the last available ID has been handed out to clients.

So by observing the rate at which IDs are being consumed and the time it takes for block allocation tasks to complete it should be possible to:

prefetch ID blocks in advance
adapt the block sizes

One caveat of using dynamically allocated block sizes might be fragmentation; if there's multiple application nodes they still need to use a consistent strategy - should not be a problem when it's based on sequences (atomic) but if it's backed by some of the other models (table?) this is probably not a good idea.

gavinking · 2021-04-15T12:49:38Z

@stuartwdouglas can you do us a favor and test this patch in the context of the Quarkus issue?

DavideD · 2021-04-15T17:04:09Z

I think we need good tests for this area

Well, yeah, clearly. The problem is that we have to precisely no tests for concurrent operation in the HR project, and I'm not clear even on the right way to write them.

I think I have an idea, I will start prototyping something

stuartwdouglas · 2021-04-15T23:00:26Z

This works now, instead of doing a couple of requests then hanging it can now handle 3.5k inserts per second (this is not tuned in any way, it could probably do a lot more if I messed with some settings).

gavinking · 2021-04-15T23:11:12Z

Thanks 🙏

So perhaps it's worth doing a release just to fix this one thing. Thoughts?

stuartwdouglas · 2021-04-15T23:23:40Z

I think so, its a pretty bad bug.

stuartwdouglas · 2021-04-15T23:24:55Z

Actually I just realized that I am only testing the block size == 1 path, how do I configure a larger block size to test the other code path?

gavinking · 2021-04-16T07:22:20Z

With the @SequenceGenerator or @TableGenerator annotation just like in regular JPA.

DavideD · 2021-04-16T07:25:47Z

So perhaps it's worth doing a release just to fix this one thing. Thoughts?

Yes, we should release it. I will work on adding some more tests for the id generation but considering that @stuartwdouglas already tested on quarkus and that our current test suite works, releasing this version seems an important improvement for a release.

stuartwdouglas · 2021-04-16T09:41:33Z

It has been more than 10 years since I last actually 'used' JPA rather than just dealing with integration stuff. So it looks like the quickstart sets allocationSize = 1 for some reason, if you remove it then performance increases, but we get the occasional PK violation:

  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     2.86ms   17.38ms 219.92ms   98.11%
    Req/Sec     8.11k     1.49k    9.41k    90.86%
  159144 requests in 10.10s, 15.69MB read
  Non-2xx or 3xx responses: 31

Looking at the logs some entities are being given the same PK.

gavinking · 2021-04-16T09:45:17Z

we get the occasional PK violation

is this with a sequence generator or a table generator?

stuartwdouglas · 2021-04-16T09:45:48Z

    @SequenceGenerator(name = "fruitsSequence", sequenceName = "known_fruits_id_seq", initialValue = 10)

gavinking · 2021-04-16T09:48:34Z

Well that's very strange. Can you tell if it's because they are assigned (a) dupe hi values or (b) dupe lo values?

gavinking · 2021-04-16T09:51:33Z

I mean:

a dupe hi value is impossible because we're getting them from a database sequence, but
a dupe lo value also looks impossible because lo values are assigned within a synchronized block.

So ... another silly logical error in my code??

stuartwdouglas · 2021-04-16T09:52:39Z

The id's in the errors are a long way apart, e.g:
5161
6361
8561
13211

So it's not allocating duplicate blocks.

stuartwdouglas · 2021-04-16T09:52:58Z

They all seem to end in '1'

gavinking · 2021-04-16T09:53:12Z

another silly logical error in my code

Yup it's that, actually. Hold tight I will push a fix.

gavinking · 2021-04-16T09:53:49Z

wait, all the ids, or just all the dupe ids?

gavinking · 2021-04-16T09:57:28Z

Yup it's that, actually. Hold tight I will push a fix.

No, excuse me. What I thought was wrong is actually fine.

gavinking · 2021-04-16T09:58:30Z

wait, all the ids, or just all the dupe ids?

No, you mean the ones in the errors. So by the looks, they're all the "second id generated after fetching a new hi value".

gavinking · 2021-04-16T10:07:29Z

they're all the "second id generated after fetching a new hi value".

No, wait I guess you have initialValue=1, the default, so they're the first id's in their respective blocks.

stuartwdouglas · 2021-04-16T10:15:00Z

I had to step away and have dinner, but my guess is something to do with reentrant calls, so the sync block doesn't help. I can't see how though.

…

On Fri, 16 Apr 2021, 8:07 pm Gavin King, ***@***.***> wrote: they're all the "second id generated after fetching a new hi value". No, wait I guess you have initialValue=1, the default, so they're the *first* id's in their respective blocks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#708 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACQG65CYQTOC7W5FYSN7CLTJAD7DANCNFSM427HLVCA> .

gavinking · 2021-04-16T10:19:58Z

Do you think it could be an out-of-order execution thing??

Like, list.forEach(Runnable::run) gets executed before result.complete( next(id) ).

Or am I trying to make this too complicated again?

gavinking · 2021-04-16T10:23:01Z

I mean, would moving the line list.forEach(Runnable::run) inside the synchronized block fix it?

stuartwdouglas · 2021-04-16T10:24:34Z

Bugger, turns out this was user error. I forgot to clean before running quarkus:dev so it had cached the old maven dependencies, so it was not picking up the new version. It looks like this was a problem with the old version when the block size was larger than 1.

Sorry for the noise.

gavinking · 2021-04-16T10:27:25Z

No problem. So how does it perform?

stuartwdouglas · 2021-04-16T13:40:55Z

Comparatively way better, 13,429 inserts per sec. This is no no way a proper benchmark, it's only using 2 threads and 10 connections.

gavinking · 2021-04-16T14:20:52Z

Alright. So that sounds good.

Shall I merge this?

DavideD · 2021-04-16T14:50:15Z

Sounds good to me

DavideD · 2021-04-16T15:46:28Z

I've released it and sent a PR for quarkus: quarkusio/quarkus#16588

DavideD · 2021-04-16T15:46:47Z

Thanks a lot @stuartwdouglas

gavinking linked an issue Apr 15, 2021 that may be closed by this pull request

BlockingIdentifierGenerator doesn't use the right session #707

Closed

proposed fix for hibernate#707

0f1d292

make sure that queued requests use the right session and entity args

gavinking force-pushed the 707 branch from 0882fae to 0f1d292 Compare April 15, 2021 11:13

when we're not using blocking, just go direct to the database

09f2d74

related to discussion around hibernate#707 This seems right for sequences. I'm not sure about tables though.

gavinking commented Apr 15, 2021

View reviewed changes

gavinking merged commit d27f7d7 into hibernate:main Apr 16, 2021

gavinking deleted the 707 branch June 15, 2023 13:41

proposed fix for #707 #708

proposed fix for #707 #708

Conversation

gavinking commented Apr 15, 2021

gavinking commented Apr 15, 2021

Sanne commented Apr 15, 2021

gavinking commented Apr 15, 2021 • edited Loading

gavinking commented Apr 15, 2021

stuartwdouglas commented Apr 15, 2021

gavinking commented Apr 15, 2021

gavinking commented Apr 15, 2021

gavinking commented Apr 15, 2021 • edited Loading

gavinking Apr 15, 2021

Choose a reason for hiding this comment

Sanne commented Apr 15, 2021

gavinking commented Apr 15, 2021

Sanne commented Apr 15, 2021

gavinking commented Apr 15, 2021

DavideD commented Apr 15, 2021

stuartwdouglas commented Apr 15, 2021

gavinking commented Apr 15, 2021

stuartwdouglas commented Apr 15, 2021

stuartwdouglas commented Apr 15, 2021

gavinking commented Apr 16, 2021

DavideD commented Apr 16, 2021

stuartwdouglas commented Apr 16, 2021

gavinking commented Apr 16, 2021

stuartwdouglas commented Apr 16, 2021

gavinking commented Apr 16, 2021 • edited Loading

gavinking commented Apr 16, 2021

stuartwdouglas commented Apr 16, 2021

stuartwdouglas commented Apr 16, 2021

gavinking commented Apr 16, 2021

gavinking commented Apr 16, 2021

gavinking commented Apr 16, 2021

gavinking commented Apr 16, 2021

gavinking commented Apr 16, 2021

stuartwdouglas commented Apr 16, 2021 via email

gavinking commented Apr 16, 2021

gavinking commented Apr 16, 2021

stuartwdouglas commented Apr 16, 2021

gavinking commented Apr 16, 2021

stuartwdouglas commented Apr 16, 2021

gavinking commented Apr 16, 2021

DavideD commented Apr 16, 2021

DavideD commented Apr 16, 2021

DavideD commented Apr 16, 2021

gavinking commented Apr 15, 2021 •

edited

Loading

gavinking commented Apr 15, 2021 •

edited

Loading

gavinking commented Apr 16, 2021 •

edited

Loading