You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Sébastien Brisard <se...@m4x.org> on 2012/09/10 21:08:42 UTC
[math] Is this good practice?
Hi,
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?
Best regards,
Sébastien
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Is this good practice?
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
Hello.
> > [...]
> 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.
Even if the actual type can be inferred at compile time, Java will not call
the method with the most specific argument type (IIRC):
ArrayFieldVector a = new ArrayFieldVector();
ArrayFieldVector b = new ArrayFieldVector();
FieldVector c = b;
FieldVector f = a.add(b); // Will call "add(ArrayFieldVector)".
FieldVector g = a.add(c); // Will call "add(FieldVector)".
I.e. what counts is the declared type.
This was the reason for having several add methods: if the declared type is
"ArrayFieldVector", you gain one method call and/or you avoid the type check
(instanceof).
> But I'm not sure the gain is
> worth the pain (again, the JIT seems to be pretty smart).
If indeed the JIT compiler inlines everything (i.e. runtime analysis having
shown which code path is actually run), then the nominal gain of one method
call disappears (IIUC).
If so, it would indeed be cleaner and more maintainable to remove the
specific method (moving the specific code to the other one as you propose).
[And it would also be more in accord with the best practice of programming
by interfaces whereas, in Java, one had a strong incentive (performance) not
to follow it. Maybe the contradiction doesn't exists anymore.]
Best regards,
Gilles
> 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-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Is this good practice?
Posted by Sébastien Brisard <se...@m4x.org>.
Hi,
2012/9/11 Gilles Sadowski <gi...@harfang.homelinux.org>:
> On Mon, Sep 10, 2012 at 10:07:11PM +0200, Benedikt Ritter wrote:
>> Hi,
>>
>> 2012/9/10 Luc Maisonobe <Lu...@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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Is this good practice?
Posted by Gilles Sadowski <gi...@harfang.homelinux.org>.
On Mon, Sep 10, 2012 at 10:07:11PM +0200, Benedikt Ritter wrote:
> Hi,
>
> 2012/9/10 Luc Maisonobe <Lu...@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).
Regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Is this good practice?
Posted by Benedikt Ritter <be...@gmail.com>.
Hi,
2012/9/10 Luc Maisonobe <Lu...@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.
Benedikt
> best regards,
> Luc
>
>> Best regards,
>> Sébastien
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [math] Is this good practice?
Posted by Luc Maisonobe <Lu...@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.
best regards,
Luc
> Best regards,
> Sébastien
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org