Hi,
2012/9/11 Gilles Sadowski <gil...@harfang.homelinux.org>:
> On Mon, Sep 10, 2012 at 10:07:11PM +0200, Benedikt Ritter wrote:
>> Hi,
>>
>> 2012/9/10 Luc Maisonobe <luc.maison...@free.fr>:
>> > Le 10/09/2012 21:08, Sébastien Brisard a écrit :
>> >> Hi,
>> >
>> > Hi Sébastien,
>> >
>> >> I thought it was not good practice to rely on exception in
>> >> unexceptional circumstances. In ArrayFieldVector, there are numerous
>> >> occurences of the following pattern
>> >>
>> >> public FieldVector<T> add(FieldVector<T> v)
>> >> throws DimensionMismatchException {
>> >> try {
>> >> return add((ArrayFieldVector<T>) v);
>> >> } catch (ClassCastException cce) {
>> >> checkVectorDimensions(v);
>> >> T[] out = buildArray(data.length);
>> >> for (int i = 0; i < data.length; i++) {
>> >> out[i] = data[i].add(v.getEntry(i));
>> >> }
>> >> return new ArrayFieldVector<T>(field, out, false);
>> >> }
>> >> }
>> >>
>> >> The "catch (ClassCastException cce)" seems uggly to me. Should I file
>> >> a JIRA issue and start replacing with instanceof?
>> >
>> > This was done on purpose a long time ago.
>> > At that time, it appeared that it was faster to do it this way, i.e.
>> > attempt by default the fast method which did not rely on getEntry, and
>> > fall back to a loop using getEntry only in the very rare cases the first
>> > fails.
>> >
>> > I am not sure this is useful anymore, as JVM optimizers are better and
>> > better (so they may well replace the getEntry on the fly and use direct
>> > array access when they can).
>> >
>> > Perhaps you could do some benchmark with a modern JVM and look if this
>> > hack is still useful or not.
>> >
>>
>> If it still is, a comment should be added to make future maintainers
>> aware of this fact.
>
> I imagine that at the time this was coded, it was assumed that it would be
> faster because it skips the conditional (the change proposed by Sébastien).
> IIUC, what Luc proposes is to check whether the JIT compiler will replace
> calls to "getEntry" with the method from the actual type. If that's true, it
> would also be true that the conditional "if (v instanceof ArrayFieldVector)"
> will be optimized away.
> Hence I'd assume that the proposed change is a safe first step (removing
> usage of exception for control flow).
>
I will try and do some benchmarking on this issue. I do not like these
"micro-optimizations" which tend to be true optimizations with one
version of the JVM, and are no longer with the next one.
Also, while we are at it, is there a benefit in defining a separate,
specialized method add(ArrayFieldVector<T>). The other option would be
to inline this method directly in the instanceof test in the general
add(FieldVector<T>). I guess the second solution would incur a slight
overhead in those cases when the compiler can resolve the actual type
of the parameter at compilation time. But I'm not sure the gain is
worth the pain (again, the JIT seems to be pretty smart). The drawback
of having two separate methods is that there is a little bit of
maintenance work in order to ensure that both have the same contract
(regarding, guess what, exceptions!). This is not a problem with the
general version of the method, which is specified in an
interface/abstract class. This is more problematic with the specific
version of the method. Of course, this only requires to be careful,
it's not so much of a hassle.
Any thoughts on this?
Sébastien
>
> Regards,
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
