Re: [bv-dev] Do we need ConstraintValidatorFactory#releaseInstance()?

2012/12/4 Emmanuel Bernard <emmanuel(a)hibernate.org&gt; > Thanks you for the sum up. > > This email is not a rebuttal, more a set of remarks on the subject. > > If we manage to remove `releaseInstance()` then we should also remove > `ValidatorFactory.close()`. AFAIR, the only reason for `close()` to > exist is to allow proper implementations of releaseInstance. > > The ConstraintValidatorFactory contract is a public contract that can be > implemented by anyone. This is also the way for DI framework to provide > injectable `ConstraintValidator` implementations. > > In my mind and in retrospect, `ConstraintValidatorFactory#getInstance()` > should have been named `createInstance()`. Remember, caching is not > allowed in a `CosntraintValidatorFactory` implementation. So you > argument that what creates an object should destroy it applies here: > what requests an object creation should request its destruction and > that's exactly why we have added this method. > > >From what you guys say, all implementors of object lifecycle management > libs (let's say DI frameworks) have a @PreDestroy logic that is called > when the CVF bean would be destroyed by its creator. It seems though > that the @PreDestroy method would have to process all objects > returned by `getInstance()`. So `getInstance` would need to keep track > of them in a concurrent structure of some sort. > Is that significantly better than having a `releaseInstance` contract? > > Pure JSR-330 implementations have no @PreDestroy concept. Nor does Guice > AFAIK. > > Today a Bean Validation implementation can cache ConstraintValidator and > discard them upon memory pressure or if they are never used. If the > @PreDestroy pattern is used, then this optimization is useless as the > object would not be garbage collected until after the CVF is explicitly > destroyed. > > It is possible to pass a ConstraintValidatorFactory instance for a > specific validator instance > > Validator validator = validatorFactory > .usingContext() > .constraintValidatorFactory(someCVFInstance) > .getValidator(); > > In this case and according to the current draft of BV 1.1, the Bean > Validation provider will likely call `getInstance` and `releaseInstance` > for each validate* calls on the `Validator`. It could technically keep > the custom CVF instance around to check if it is reused in a subsequent > `Validator` creation. > > Because of: > > - the custom CVF instance per Validator > - the ability to offload cached ConstraintValidator instances > - the ability to implement a dump BV provider that caches nothing > > I added what turned out to be a mysterious provision in the spec > > > `releaseInstance()` is "typically" invoked when the ValidatorFactory is > > closed > > So from what I understand, the choice is between: > > - explicit releaseInstance and VF.close() with the benefit of more > optimizations wrt memory usage - at least not involving custom weak > reference like implementations > - rely on the @PreDestroy approach - thus limiting CVF implementations > with such > feature - bringing simplicity to the BV provider implementations and > removing VF.close() > Yes, I think that's basically the two options we have. I'm still not sure whether we really do gain much by releaseInstance(). I think the argument for it is that it allows for releasing validators upon memory shortage *before* ValidatorFactory#close() is called. But when and how would a BV provider make use of this? Right now I can see two possible implementation approaches: 1) A BV provider uses a fixed-size cache of validators and calls CVF#releaseInstance() on an LRU basis for recently unused validators. But how would this cache be sized? I think it could easily be either too small (causing new validators to be created unnecessarily) or too large (causing memory issues). 2) A BV provider uses a Reference based cache, utilizing as much memory as possible. In case of memory shortage the provider would call releaseInstance() for freed instances. To me, option 1) seems not so good due to the sizing issue, so I'd personally go for option 2). But if a BV provider has to deal with References anyways, wouldn't it be simpler to do the same in CVF implementations (properly disposing validators once they're freed)? By using Reference based caches in CVF and BV provider, I think there would also be no problem for frameworks which don't offer a shutdown hook, as validators would be released as required. All in all, to me the simplified contract between CVF and BV provider by removing releaseInstance() seems like the better option.