make ceylon.language::sum fast #728
Conversation
This implementation avoids using a boxed type for the `sum` accumulator variable when summing Integers and Floats.
|
Hmm I should test how my experimental boxing/unboxing code handles this. Because although it's a clever trick what you are doing here it depends completely on implementation details that could change at any time. |
|
I beg to differ.
For the last item, certainly a compiler improvement would be to treat Again, for the last item, boxing logic could be improved such that only the accumulator would need to explicitly be an |
|
Minor addition: I suppose pure |
|
I also beg to differ with your point 3, you don't get to decide what the compiler should always do, that's the compiler's business :) |
|
Ok, yeah, I agree that it looks ugly. |
|
Sorry, it's not the accumulator that "bothers" me, it's the |
|
@jvasileff I don't love this implementation, but the performance results are significant. My feeling is that if we're going to do an optimization like this I would prefer to do it by making |
|
P.S. I agree that this hack: Should not be necessary and that the Java backend should recognize that |
|
|
Just a question @gavinking , but why is this an issue for the compiler? Couldn't the typechecker just immediately simplify a |
|
@quintesse No, of course not. |
|
I mean |
|
AFAICT, a native implementation would:
But I'm not seeing further optimization opportunities. The second item, while significant for small streams, may also be eliminated with ceylon/ceylon-spec#1399 and ceylon/ceylon-compiler#2270 . Some results: for: package sandbox.ceylon.snap.base;
import com.redhat.ceylon.compiler.java.metadata.Ignore;
import com.redhat.ceylon.compiler.java.metadata.Name;
import com.redhat.ceylon.compiler.java.metadata.TypeInfo;
import com.redhat.ceylon.compiler.java.metadata.TypeParameter;
import com.redhat.ceylon.compiler.java.metadata.TypeParameters;
import com.redhat.ceylon.compiler.java.metadata.Variance;
import com.redhat.ceylon.compiler.java.runtime.model.TypeDescriptor;
import ceylon.language.Float;
import ceylon.language.Integer;
import ceylon.language.Iterable;
import ceylon.language.Iterator;
import ceylon.language.SharedAnnotation$annotation$;
import ceylon.language.Summable;
@com.redhat.ceylon.compiler.java.metadata.Ceylon(major = 8)
@com.redhat.ceylon.compiler.java.metadata.Method
public class nativeJavaSum_ {
private nativeJavaSum_() {
}
@SharedAnnotation$annotation$
@TypeInfo(
value = "Value",
erased = true)
@TypeParameters({@TypeParameter(
value = "Value",
variance = Variance.NONE,
satisfies = {"ceylon.language::Summable<Value>"},
caseTypes = {})})
public static <Value extends Summable<Value>>Value nativeJavaSum(@Ignore
final TypeDescriptor $reified$Value, @Name("values")
@TypeInfo(
value = "{Value+}",
erased = true)
final Iterable<? extends Value, ? extends Object> values) {
final Iterator<? extends Value> it = values.iterator();
final Object first = it.next();
if (first instanceof Integer) {
long sum = ((Integer)first).longValue();
while (true) {
Object val$670;
if ((val$670 = it.next()) instanceof Integer) {
sum += ((Integer)val$670).longValue();
} else {
break;
}
}
return (Value)Integer.instance(sum);
} else {
if (first instanceof Float) {
double sum = ((Float)first).doubleValue();
while (true) {
Object val$665;
if ((val$665 = it.next()) instanceof Float) {
sum += ((Float)val$665).doubleValue();
} else {
break;
}
}
return (Value) Float.instance(sum);
} else {
throw new UnsupportedOperationException();
}
}
}
} |
|
Just to throw something extra out there, it seems to me that this (slightly ugly) implementation using shared Value sum2<Value>({Value+} values)
given Value satisfies Summable<Value> {
variable value sum = values.first;
variable value first = true;
values.each(void (element) {
if (first) {
sum = element;
first = false;
}
else {
sum+=element;
}
});
return sum;
} |
|
Scratch that: with a correct implementation it doesn't seem to make much difference. However this implementation does seem to be faster: shared Value sum4<Value>({Value+} values)
given Value satisfies Summable<Value>
=> values.reduce(plus);... at least in some cases. |
|
Yeah, that makes sense, since there is an optimized implementation of |
|
if (is Integer first) {
// unbox; don't infer type Value&Integer
variable Integer sum = 0;
values.each(void (item) {
assert (is Integer item);
Integer unboxed = item;
sum += unboxed;
});
assert (is Value result = sum);
return result;
}but the overhead kills performance for and of course this ugly impl reads the first element twice, so it isn't really an option anyway. But for I wasn't able to see an advantage with |
|
|
|
Ok, weird. print(type(ints));
// ceylon.language::ArraySequence<ceylon.language::Integer>But hotspot is a strange beast. |
this is based on code + ideas by @jvasileff in #728, but using native
|
Apologies, @jvasileff, I have pushed a copy/paste of your code instead of merging this PR because I wanted to make it a |
|
P.S. @jvasileff thanks for the contribution! |
|
No problem at all. Looks like the approach saved us both some time. |
This implementation avoids using a boxed type for the
sumaccumulator variable when summingIntegers andFloats.If you want to merge this, I can also make the change for
product.Benchmark Script
Integer Results
Float Results