Remove casts and boxing for dynamic math by rmuir · Pull Request #18849 · elastic/elasticsearch

rmuir · 2016-06-14T04:56:53Z

#18847 gave us the ability to optimize this stuff when we know types, but we don't use it yet.

This patch removes the manual boxing/promotion/conversion and just hands over what we have for the binary math and comparison operators. For example in the script in the nightly benchmark:

(Math.log(Math.abs(doc['population'].value)) + doc['elevation'].value * doc['latitude'].value)/_score

We remove two calls to Double.valueOf: one because we know the _score is a double argument, we don't need to box it, and another because we know the math function returns a double.

Instead of:

    ALOAD 2
    INVOKEVIRTUAL org/apache/lucene/search/Scorer.score ()F
    F2D
    DSTORE 5
    ALOAD 3
    LDC "population"
    INVOKEINTERFACE java/util/Map.get (Ljava/lang/Object;)Ljava/lang/Object;
    INVOKEDYNAMIC value(Ljava/lang/Object;)D [...]
    INVOKESTATIC java/lang/Math.abs (D)D
    INVOKESTATIC java/lang/Math.log (D)D
    INVOKESTATIC java/lang/Double.valueOf (D)Ljava/lang/Double;
    ALOAD 3
    LDC "elevation"
    INVOKEINTERFACE java/util/Map.get (Ljava/lang/Object;)Ljava/lang/Object;
    INVOKEDYNAMIC value(Ljava/lang/Object;)Ljava/lang/Object; [...]
    ALOAD 3
    LDC "latitude"
    INVOKEINTERFACE java/util/Map.get (Ljava/lang/Object;)Ljava/lang/Object;
    INVOKEDYNAMIC value(Ljava/lang/Object;)Ljava/lang/Object; [...]
    INVOKEDYNAMIC mul(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object; [...]
    INVOKEDYNAMIC add(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object; [...]
    DLOAD 5
    INVOKESTATIC java/lang/Double.valueOf (D)Ljava/lang/Double;
    INVOKEDYNAMIC div(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object; [...]
    ARETURN

We do:

    ALOAD 2
    INVOKEVIRTUAL org/apache/lucene/search/Scorer.score ()F
    F2D
    DSTORE 5
    ALOAD 3
    LDC "population"
    INVOKEINTERFACE java/util/Map.get (Ljava/lang/Object;)Ljava/lang/Object;
    INVOKEDYNAMIC value(Ljava/lang/Object;)D [ ... ]
    INVOKESTATIC java/lang/Math.abs (D)D
    INVOKESTATIC java/lang/Math.log (D)D
    ALOAD 3
    LDC "elevation"
    INVOKEINTERFACE java/util/Map.get (Ljava/lang/Object;)Ljava/lang/Object;
    INVOKEDYNAMIC value(Ljava/lang/Object;)Ljava/lang/Object; [ ... ]
    ALOAD 3
    LDC "latitude"
    INVOKEINTERFACE java/util/Map.get (Ljava/lang/Object;)Ljava/lang/Object;
    INVOKEDYNAMIC value(Ljava/lang/Object;)Ljava/lang/Object; [ ... ]
    INVOKEDYNAMIC mul(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object; [ ... ]
    INVOKEDYNAMIC add(DLjava/lang/Object;)Ljava/lang/Object; [ ... ]
    DLOAD 5
    INVOKEDYNAMIC div(Ljava/lang/Object;D)Ljava/lang/Object; [ ... ]
    ARETURN

I added lots of tests to DefOptimizationTests for parameter and return values, and also correctness tests to DefOperationsTests for when we have partial types. I also found and fixed some bugs along the way (e.g. EComp did not work correctly when only one operand was def, it only looked at the RHS).

rmuir · 2016-06-14T05:04:36Z

With this patch, i see only small differences in that nightly benchmark between static and dynamic now, typically something like this:

SEARCH painless_static (median): 0.438902 sec
SEARCH painless_dynamic (median): 0.459875 sec

uschindler · 2016-06-14T08:25:13Z

With this patch, i see only small differences in that nightly benchmark between static and dynamic now

How was it before?

uschindler · 2016-06-14T08:27:33Z

In general I have to closer look into the code!

I know, we cannot use the IDefLink class here, as the operands are SExpression not ALink, so we need a lot of if then else. But the current code just looking for Sort.DEF is fine (as simple). The IDefLink is more needed for the inverse case (links that make an expression at end).

Maybe we find a better solution for the def rewriting, there is room for improvement. As a fisrt step this looks good!

rmuir · 2016-06-14T11:33:34Z

With this patch, i see only small differences in that nightly benchmark between static and dynamic now

How was it before?

Before typically like a 15% difference.

In general I have to closer look into the code!

I know, we cannot use the IDefLink class here, as the operands are SExpression not ALink, so we need a lot of if then else. But the current code just looking for Sort.DEF is fine (as simple). The IDefLink is more needed for the inverse case (links that make an expression at end).

These E-nodes are different than that case. In this case we just want to not generate the casts and boxing, and instead take the types and expected return values as-is and pass to invokedynamic, thats what the change does.

If we want to reduce if-then-else (this is pre-existing, from the static case, i am afraid to heavily refactor it here), then the easiest way to do that, would be to refactor nodes to have the same behavior. For example shifts have different promotion than additions, but they are all bundled in one EBinary.

Maybe we find a better solution for the def rewriting, there is room for improvement. As a fisrt step this looks good!

I don't really see it as rewriting the node, as it doesnt do that. It just inhibits the explicit casts and boxing. For these cases it can be done on return value and arguments without anything heavy.

Some cases are still to be improved. For example compound assignments in EChain. But see my comment, i think thats too scary to optimize right now as i see bugs looking at it, we need tons of tests for those compound assignments first...

uschindler · 2016-06-14T13:20:51Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/MethodWriter.java


+    /** Writes a dynamic binary instruction: returnType, lhs, and rhs can be different */
+    public void writeDynamicBinaryInstruction(Location location, Type returnType, Type lhs, Type rhs, Operation operation) {
+        org.objectweb.asm.Type descriptor = org.objectweb.asm.Type.getMethodType(returnType.type, lhs.type, rhs.type);


I would change the name here to somthing like methodType.
In addition: This looks like messy code duplication. Maybe just use a simple wrapper method for the invokeDynamic, to not copy paste each line?

Its not easy to refactor without it being trappy. Remember, not all of these are 'binary' instructions. Some are shifts. so each guy needs to set at least 2 things, what type he is and what instruction.

To me, we simply have to stop categorizing all of these as the same thing. They are very different operators. It causes all the if-then-else.

Also the Operation enum we have is currently not helpful here to address these issues, because it is more of a syntax level thing. It only has one - for example. It does not know about any semantics.

I don't plan to change this on the issue. Please look at the changes, it used a switch on invokedynamic before this PR. Its not my fault. Lets fix it separate.

OK. It was just a suggestion. UweSays: Switch statements are very non-java like and must die :-)

I know, i am just exhausted from the slave labor on this one already. I did try to do the refactorings you suggested, to try to make it cleaner and also remove string constants and stuff. But it got messy quickly (the root problem, Operator is syntax oriented).

I'm not gonna leave it alone. I just want to iterate here. There is a lot to followup with. For example return values of unary operators, compound assignments, all other cases where we do inefficient casts.

jdconrad · 2016-06-14T16:43:51Z

I'm +1 on these changes. Great use of the extra type information.

uschindler · 2016-06-14T16:47:41Z

+1 also from my side!

rmuir · 2016-06-14T17:01:35Z

I'm gonna followup by splitting up that massive DefOperations test too. We should have it for each operator. Some stuff is still missing like compound assignment tests and there are bugs in those for def!!!!

painless: remove casts and boxing for dynamic math

a12937d

rmuir added :Plugin Lang Painless v5.0.0-alpha4 labels Jun 14, 2016

clintongormley added the >enhancement label Jun 14, 2016

clintongormley changed the title ~~painless: remove casts and boxing for dynamic math~~ Remove casts and boxing for dynamic math Jun 14, 2016

uschindler reviewed Jun 14, 2016
View reviewed changes

rename this variable

79f0ab2

rmuir merged commit e4dc469 into elastic:master Jun 14, 2016

jdconrad mentioned this pull request Jun 14, 2016

Ongoing Painless Improvements #17992

Closed

18 tasks

rmuir mentioned this pull request Jun 14, 2016

improve unary operators and cleanup tests #18867

Merged

clintongormley added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache and removed :Plugin Lang Painless labels Feb 14, 2018

Conversation

rmuir commented Jun 14, 2016

Uh oh!

rmuir commented Jun 14, 2016

Uh oh!

uschindler commented Jun 14, 2016

Uh oh!

uschindler commented Jun 14, 2016

Uh oh!

rmuir commented Jun 14, 2016

Uh oh!

uschindler Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

rmuir Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

rmuir Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

rmuir Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

uschindler Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

rmuir Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

jdconrad commented Jun 14, 2016

Uh oh!

uschindler commented Jun 14, 2016

Uh oh!

rmuir commented Jun 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants