On 09/27/2016 01:35 PM, Sanne Grinovero wrote: > Hibernate OGM is using the FGAMs and needs their "fine-grained" > semantics as it's being used to store what is perceived possibly as > independent user entries; having these entries share a same lock would > introduce possibilities of deadlock depending on the end user's > business logic; we're an abstraction layer and these semantics are > being relied on. > I believe a good analogy to this is the comparison of databases > implementing row-level locking vs. page-level locking; such changes in > granularity would make end users loose hair so I'll not change our > usage from FGAMS to AMs.
I am not asking you to move from FGAMs to AMs as-is - I am trying to precisely asses your needs and provide an option that suits them best, while not creating malfunctioning configurations and breaking encapsulation. OGM uses <transaction lockingMode="OPTIMISTIC" ... /> configuration by default (not sure if you support changing that to pessimistic). With this configuration, there's no risk of deadlocks within Infinispan, no matter what order you use to read/write entries in a transaction. All locks are acquired in a deterministic order only during transaction commit, that appears to OGM (or the user) as a single atomic operation. So what fine-grained gives you now is that you don't have to wait until the tx1.commit() call completes before tx2.commit() can start. So I see the performance impact upon highly concurrent modification of single entry, but from the end-user perspective it should not have any visible effect (least lose hair :)). Another important feature of AtomicMaps (but FGAM and AM have it in common) is that they send only the delta over the wire - such behaviour must be preserved, of course, but functional API should supersede the hacks in place now. > We can maybe move to grouping or to queries; this wasn't considered > back then as neither grouping nor (unindexed) queries were available. > Both grouping and queries have their drawbacks though, so we can take > this in consideration on the OGM team but it's going to take some > time; > > So feel free deprecate FGAMs but please don't remove them yet. We wouldn't remove the interface in 9.0; we would return something that does what you need but differently. > > For the record, OGM never uses write skew checks on FGAMs, and also > Hibernate OGM / Hot Rod doesn't use FGAMs (whe use them only in > embedded mode, when transactions are available). And that's the only reason why it was not fixed sooner :) Radim > > Thanks, > Sanne > > > On 27 September 2016 at 08:28, Radim Vansa <rva...@redhat.com> wrote: >> To Pedro: I have figured out that it shouldn't work rather >> theoretically, so I haven't crashed into ISPN-2729. >> >> On 09/26/2016 10:51 PM, Dan Berindei wrote: >>> On Mon, Sep 26, 2016 at 10:36 AM, Radim Vansa <rva...@redhat.com> wrote: >>>> Hi all, >>>> >>>> I have realized that fine grained maps don't work reliably with >>>> write-skew check. This happens because WSC tries to load the entry from >>>> DC/cache-store, compare versions and store it, assuming that this >>>> happens atomically as the entry is locked. However, as fine grained maps >>>> can lock two different keys and modify the same entry, there is a risk >>>> that the check & store won't be atomic. Right now, the update itself >>>> won't be lost, because fine grained maps use DeltaAwareCacheEntries >>>> which apply the updates DC's lock (there can be some problems when >>>> passivation is used, though, [1] hopefully deals with them). >>>> >>> I had a hard time understanding what the problem is, but then I >>> realized it's because I was assuming we keep a separate version for >>> each subkey. After I realized it's not implemented like that, I also >>> found a couple of bugs I filed for it a long time ago: >>> >>> https://issues.jboss.org/browse/ISPN-3123 >>> https://issues.jboss.org/browse/ISPN-5584 >>> >>>> I have figured this out when trying to update the DeltaAware handling to >>>> support more than just atomic maps - yes, there are special branches for >>>> atomic maps in the code, which is quite ugly design-wise, IMO. My >>>> intention is to do similar things like WSC for replaying the deltas, but >>>> this, obviously, needs some atomicity. >>>> >>> Yes, for all the bugs in the AtomicMaps, it's even harder implementing >>> a DeltaAware that is not an AtomicMap... >>> >>> But I don't see any reason to do that anyway, I'd rather work on >>> making the functional stuff work with transactions. >> Yes, I would rather focus on functional stuff too, but the Delta* stuff >> gets into my way all the time, so I was trying to remove that. However, >> though we could deprecate fine grained maps (+1!) we have to keep it >> working as OGM uses that. I am awaiting some details from Sanne that >> could suggest alternative solution, but he's on PTO now. >> >>>> IIUC, fine-grained locking was introduced back in 5.1 because of >>>> deadlocks in the lock-acquisition algorithm; the purpose was not to >>>> improve concurrency. Luckily, the days of deadlocks are far back, now we >>>> can get the cluster stuck in more complex ways :) Therefore, with a >>>> correctness-first approach, in optimistic caches I would lock just the >>>> main key (not the composite keys). The prepare-commit should be quite >>>> fast anyway, and I don't see how this could affect users >>>> (counter-examples are welcome) but slightly reduced concurrency. >>>> >>> I don't remember what initial use case for FineGrainedAtomicMaps was, >>> but I agree with Pedro that it's a bit long in the tooth now. The only >>> advantage of FGAM over grouping is that getGroup(key) needs to iterate >>> over the entire data container/store, so it can be a lot slower when >>> you have lots of small groups. But if you need to work with all the >>> subkeys in the every transaction, you should probably be using a >>> regular AtomicMap instead. >> Iterating through whole container seems like a very limiting factor to >> me, but I would keep AtomicMaps and let them be implemented through >> deltas/functional commands (preferred), but use the standard locking >> mechanisms instead of fine-grained insanity. >> >>> IMO we should deprecate FineGrainedAtomicMap and implement it as a >>> regular AtomicMap. >>> >>>> In pessimistic caches we have to be more cautious, since users >>>> manipulate the locks directly and reason about them more. Therefore, we >>>> need to lock the composite keys during transaction runtime, but in >>>> addition to that, during the commit itself we should lock the main key >>>> for the duration of the commit if necessary - pessimistic caches don't >>>> sport WSC, but I was looking for some atomicity options for deltas. >>>> >>> -1 to implicitly locking the main key. If a DeltaAware implementation >>> wants to support partial locking, then it should take care of the >>> atomicity of the merge operation itself. If it doesn't want to support >>> partial locking, then it shouldn't use AdvancedCache.applyDelta(). >>> It's a bit unfortunate that applyDelta() looks like a method that >>> anyone can call, but it should only be called by the DeltaAware >>> implementation itself. >> As I have mentioned in my last mail, I found that it's not that easy, so >> I am not implementing that. But it's not about taking care of atomicity >> of the merge - applying merge can be synchronized, but you have to do >> that with the entry stored in DC when the entry is about to be stored in >> DC - and this is the only moment you can squeeze the WSC inl, because >> the DeltaAware can't know anything about WSCs. That's the DC locking >> piggyback you -10. >> >>> However, I agree that implementing a DeltaAware partial locking >>> correctly in all possible configurations is nigh impossible. So it >>> would be much better if we also deprecate applyDelta() and start >>> ignoring the `locksToAcquire` parameter. >>> >>>> An alternative would be to piggyback on DC's locking scheme, however, >>>> this is quite unsuitable for the optimistic case with a RPC between WSC >>>> and DC store. In addition to that, it doesn't fit into our async picture >>>> and we would send complex compute functions into the DC, instead of >>>> decoupled lock/unlock. I could also devise another layer of locking, but >>>> that's just madness. >>>> >>> -10 to piggyback on DC locking, and -100 to a new locking layer. >>> >>> I think you could lock the main key by executing a >>> LockControlCommand(CACHE_MODE_LOCAL) from >>> PessimisticLockingInterceptor.visitPrepareCommand, before passing the >>> PrepareCommand to the next interceptor. But please don't do it! >> Okay, I'll just wait until someone tells me why the heck anyone needs >> fine grained, discuss how to avoid it and then deprecate it :) >> >> Radim >> >>>> I am adding Sanne to recipients as OGM is probably the most important >>>> consumer of atomic hash maps. >>>> >>>> WDYT? >>>> >>>> Radim >>>> >>>> [1] >>>> https://github.com/infinispan/infinispan/pull/4564/commits/2eeb7efbd4e1ea3e7f45ff2b443691b78ad4ae8e >>>> >>>> -- >>>> Radim Vansa <rva...@redhat.com> >>>> JBoss Performance Team >>>> >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev@lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev@lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> -- >> Radim Vansa <rva...@redhat.com> >> JBoss Performance Team >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev@lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Radim Vansa <rva...@redhat.com> JBoss Performance Team _______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
