[mypyc] Faster min #10265

ChetanKhanna · 2021-03-30T16:05:56Z

Description

I have tried to specialize min(x, y) in this PR as per mypyc/mypyc#773

Test Plan

I ran the complete test suite locally as well as https://github.com/mypyc/mypyc-benchmarks
However, I'm still unsure as to how am I supposed to report benchmark results. After running the benchmark runner on both master and this branch for 5 time, I got the following results (averaged):
master: 11.7058x faster
current branch: 11.8452x faster

JukkaL · 2021-03-30T17:36:49Z

mypyc/irbuild/specialize.py

+            and expr.arg_kinds == [ARG_POS, ARG_POS]):
+        x, y = builder.accept(expr.args[0]), builder.accept(expr.args[1])
+        comparison = builder.binary_op(x, y, '<', expr.line)
+        if comparison == Integer(1,  bool_rprimitive):


This isn't right. The if statement runs during compilation, but it should instead generate an if statement that is executed when the compiled code is run.

I've updated this. However, I posted a question below. Any suggestions please?

JukkaL · 2021-03-30T17:38:22Z

mypyc/test-data/run-integers.test

+def check_min_int() -> None:
+    x: int = 200
+    y: int = 30
+    assert min(x, y) == 30


Also test the case where the first argument is the minimum. I think that it's going to fail.

To help debug this, also add an irbuild test case for this. This way you can validate that the code you are generating is what you expect it to be.

I get why you think that way. I've updated the logic a bit to get a better IR. I tested locally and the other case seem to work fine as well. I'll add that test case before pushing next time.

TH3CHARLie · 2021-03-31T01:36:32Z

If I recall correctly, the specializer logic would pick the first specializer of a function name. Because there is already a registered builtins.min for generator call, I doubt you actually specialize the call and the performance numbers look like noise to me. As Jukka suggested, write a simple IR test and see if the IR matches your desired case.

ChetanKhanna · 2021-03-31T16:50:47Z

Okay, I think I'll have to dig in deeper then. I'll keep on updating this thread with all the progress. Thank you :)

ChetanKhanna · 2021-04-01T15:49:19Z

Hello! Okay so I was looking into it and I have a couple of doubts:-

Writing an irbuild test helped. However, I feel there is an extra block in the output and I'm not sure how to avoid it from getting generated while compiling. Here's the output and I think block L6 shouldn't be there. If anyone could suggest how to possibly handle this please?

def f(x, y):
      x, y :: int
      r0 :: int64
      r1 :: bit
      r2 :: int64
      r3, r4, r5 :: bit
      r6 :: bool
      r7 :: bit
      r8 :: object
      r9 :: str
      r10, r11, r12, r13 :: object
      r14 :: int
  L0:
      r0 = x & 1
      r1 = r0 == 0
      r2 = y & 
      r3 = r2 == 0
      r4 = r1 & r3
      if r4 goto L1 else goto L2 :: bool
  L1:
      r5 = x < y :: signed
      r6 = r5
      goto L3
  L2:
      r7 = CPyTagged_IsLt_(x, y)
      r6 = r7
  L3:
      if r6 goto L4 else goto L5 :: bool
  L4:
      return x
  L5:
      return y
  L6:
      r8 = builtins :: module
      r9 = 'min'
      r10 = CPyObject_GetAttr(r8, r9)
      r11 = box(int, x)
      r12 = box(int, y)
      r13 = PyObject_CallFunctionObjArgs(r10, r11, r12, 0)
      r14 = unbox(int, r13)
      return r14

As @TH3CHARLie said that builtins.min is already registered and shouldn't be registered again on the new function. Then should the new function be shifted inside the translate_safe_generator_call which originally registers min?

TH3CHARLie · 2021-04-02T01:34:07Z

For Q1: I suggest you update your local commit so we can figure out why L6 appears. Your guess is correct, it shouldn't be here, it's a generic implementation that is usually slower than direct integer comparison.

For Q2: my advice is to make the specializers from a map of <name, function> to <name, list of functions> and update the lookup logic accordingly. But this seems a little bit heavy, not sure how @JukkaL thinks.

ChetanKhanna · 2021-04-03T18:57:19Z

For Q1: I suggest you update your local commit so we can figure out why L6 appears. Your guess is correct, it shouldn't be here, it's a generic implementation that is usually slower than direct integer comparison.

I've pushed my local commits, although they are still WIP

For Q2: my advice is to make the specializers from a map of <name, function> to <name, list of functions> and update the lookup logic accordingly. But this seems a little bit heavy, not sure how @JukkaL thinks.

Okay, that makes sense. I can proceed that way if that's okay.

97littleleaf11 · 2021-04-07T10:59:19Z

For Q2: my advice is to make the specializers from a map of <name, function> to <name, list of functions> and update the lookup logic accordingly. But this seems a little bit heavy, not sure how @JukkaL thinks.

Agree with this idea. Recent speed-up commits and issues have and will change a large range of code structure. Maybe we can open a new issue to discuss this?

JukkaL · 2021-04-07T19:01:20Z

Maybe we can open a new issue to discuss this?

This is a good idea. My first impression is that having a list of functions is probably fine, but it's worth thinking about it a bit more.

mypyc/test-data/irbuild-int.test

97littleleaf11 · 2021-05-09T17:13:05Z

mypyc/irbuild/specialize.py

+
+@specialize_function('builtins.min')
+def faster_min(builder: IRBuilder, expr: CallExpr, callee: RefExpr) -> None:
+    if (len(expr.args) > 0


The return value should be Optional[Value]. Then the call translation functions in irbuild/expression.py can get the correct result and stop further trying.

This was super helpful, thanks for pointing out 😄 I should have been more careful.

97littleleaf11 · 2021-07-20T19:06:38Z

lgtm! This PR brings a promising speed-up:

running min_max_pair
..........
interpreted: 0.456433s (avg of 5 iterations; stdev 3.1%)
compiled:    0.264439s (avg of 5 iterations; stdev 0.33%)

compiled is 1.726x faster

Master branch is about 0.9x.

97littleleaf11 · 2021-07-29T13:50:47Z

mypyc/test-data/run-integers.test

+
+[case testMin]
+def check_min_int() -> None:
+    x: int = 200


I think functions with prefix check wouldn't trigger a run test? Maybe you should change it to test_*. Also, please add more run tests about other types, such as strings, floats.

I'm having some issues with my system right now. I'll try to update the branch by the weekend.

ChetanKhanna · 2021-08-08T13:55:40Z

If this is good, then I can the max counterpart as well.

97littleleaf11

Thanks for contributing! I have several suggesions on testing. The specializer looks good to me and you could support max in this PR.

btw, we don't recommend force-push since it would break previous reviews.

97littleleaf11 · 2021-08-08T14:34:59Z

mypyc/test-data/run-floats.test

@@ -20,3 +20,9 @@ def test_abs() -> None:
    assert abs(44324.732) == 44324.732
    assert abs(-23.4) == 23.4
    assert abs(-43.44e-4) == 43.44e-4
+
+[case testFloatMin]
+def test_float_min() -> None:


I think merging these test cases into one might be better.

97littleleaf11 · 2021-08-08T14:41:33Z

mypyc/test-data/fixtures/ir.py

+@overload
+def min(x: float, y: float) -> float: ...
+@overload
+def min(x: str, y: str) -> str: ...


You can use T (already defined in this file) to make them simple.

97littleleaf11 · 2021-08-08T14:47:42Z

mypyc/test-data/run-strings.test

+def test_str_min() -> None:
+    x: str = 'aaa'
+    y: str = 'bbb'
+    assert min(x, y) == 'aaa'


you can also test the minimun of user defined class which has __lt__.

Test the inverse here as well.

JukkaL · 2021-08-11T10:40:10Z

Please also test heterogeneous operands for min (e.g. Any and int; also int and Any). The int operands needs to be coerced to object. Another interesting case is int and bool.

Finally, test Any and Any with a few different types, and test Any and Any with incompatible types (e.g. int and str).

ChetanKhanna · 2021-08-18T10:12:21Z

Sorry for the late update. I tried all the tests as @JukkaL suggested, they all failed to compile. I get an error like:
error: assignment to ‘CPyTagged’ {aka ‘long unsigned int’} from ‘PyObject *’ {aka ‘struct _object *’} makes integer from pointer without a cast

Pushing the commit so that everyone can see as well.

The int operands needs to be coerced to object.

Do we need to explicitly perform coercion in the new faster_min function?

ChetanKhanna · 2021-08-20T20:27:52Z

I made changes to faster_min, the tests are running fine now.

JukkaL

Left a few comments (not a full review).

mypyc/test-data/run-floats.test

JukkaL · 2021-11-10T11:43:05Z

mypyc/test-data/run-strings.test

+def test_str_min() -> None:
+    x: str = 'aaa'
+    y: str = 'bbb'
+    assert min(x, y) == 'aaa'


Test the inverse here as well.

mypyc/test-data/run-strings.test

JukkaL

Looks good! I checked that min is now much faster than previously -- it can be over 10x faster than before.

I have just a two minor comments. Feel free to merge once you've addressed them.

Extending this to support max should be pretty easy and a good follow-up PR :-)

mypyc/irbuild/specialize.py

JukkaL · 2021-11-11T16:38:30Z

mypyc/irbuild/specialize.py

+            and expr.arg_kinds == [ARG_POS, ARG_POS]):
+        x, y = builder.accept(expr.args[0]), builder.accept(expr.args[1])
+        result = Register(builder.node_type(expr))
+        comparison = builder.binary_op(x, y, '<', expr.line)


Based on a quick experiment, it seems that CPython does actually evaluate y < x instead of x < y when doing min(x, y). Can you double check this and update the implementation accordingly if that's the case? (This is not a big deal, but it's better to remain as close to CPython as possible.)

Thanks for pointing out! I checked the bltinmodule/min_max() in CPython and it does eval y < x when doing min(x, y). To be more specific, it evaluates arguments from last to first.

Description Closes mypyc/mypyc#773, follows up to #10265

Speeds up `min(x, y)` using a specializer. Co-authored-by: 97littleleaf11 <[email protected]>

Description Closes mypyc/mypyc#773, follows up to python#10265

JukkaL requested changes Mar 30, 2021

View reviewed changes

97littleleaf11 mentioned this pull request Apr 13, 2021

Consider multiple specialized function mypyc/mypyc#832

Closed

ChetanKhanna force-pushed the issue-773 branch from 03c2852 to 3a0c7b7 Compare May 9, 2021 14:42

ChetanKhanna commented May 9, 2021

View reviewed changes

mypyc/test-data/irbuild-int.test Outdated Show resolved Hide resolved

97littleleaf11 suggested changes May 9, 2021

View reviewed changes

ChetanKhanna changed the title ~~[WIP][mypyc] Faster min~~ [mypyc] Faster min May 10, 2021

97littleleaf11 suggested changes Jul 29, 2021

View reviewed changes

ChetanKhanna added 7 commits August 8, 2021 16:03

[mypyc] Faster min

ae13fd1

update faster min logic

bb0f50d

Moved faster_min to separate function and added another run test

081b205

Added ir-build test

f4d881f

Changed int64 -> native_int

803dd8f

fixed extra block bug in faster min

22b1e50

Changed test name and added more tests

62998ec

ChetanKhanna force-pushed the issue-773 branch from cedf295 to 62998ec Compare August 8, 2021 12:06

97littleleaf11 suggested changes Aug 8, 2021

View reviewed changes

ChetanKhanna added 2 commits August 11, 2021 00:59

updated tests

173ca8f

Fix bug in dunder_test

b7a7ae3

Added heterogenous min test

5b2773b

fixed heterogenous ops in faster_min

b2aecc6

97littleleaf11 added 2 commits November 10, 2021 18:40

Merge from master

96cab7d

Remove unused import

e3b78b8

JukkaL reviewed Nov 10, 2021

View reviewed changes

Add some tests

5591f59

97littleleaf11 requested a review from JukkaL November 10, 2021 12:27

97littleleaf11 mentioned this pull request Nov 11, 2021

[mypyc] Faster min max #9713

Closed

JukkaL approved these changes Nov 11, 2021

View reviewed changes

97littleleaf11 added 3 commits November 12, 2021 09:15

eval y<x when doing min(x, y)

11b9d46

Fix

ebef552

Fix IR test

8af7592

97littleleaf11 merged commit 0b4cb1e into python:master Nov 12, 2021

97littleleaf11 mentioned this pull request Nov 12, 2021

[mypyc] Faster max #11530

Merged

TH3CHARLie pushed a commit that referenced this pull request Nov 12, 2021

[mypyc] Faster max (#11530)

aeb65a6

Description Closes mypyc/mypyc#773, follows up to #10265

tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this pull request Jan 20, 2022

[mypyc] Faster min (python#10265)

40f89db

Speeds up `min(x, y)` using a specializer. Co-authored-by: 97littleleaf11 <[email protected]>

tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this pull request Jan 20, 2022

[mypyc] Faster max (python#11530)

ff540ae

Description Closes mypyc/mypyc#773, follows up to python#10265

Uh oh!

[mypyc] Faster min #10265

[mypyc] Faster min #10265

Conversation

ChetanKhanna commented Mar 30, 2021

Description

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TH3CHARLie commented Mar 31, 2021

Uh oh!

ChetanKhanna commented Mar 31, 2021

Uh oh!

ChetanKhanna commented Apr 1, 2021

Uh oh!

TH3CHARLie commented Apr 2, 2021

Uh oh!

ChetanKhanna commented Apr 3, 2021

Uh oh!

97littleleaf11 commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JukkaL commented Apr 7, 2021

Uh oh!

Uh oh!

97littleleaf11 May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

97littleleaf11 commented Jul 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChetanKhanna commented Aug 8, 2021

Uh oh!

97littleleaf11 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JukkaL commented Aug 11, 2021

Uh oh!

ChetanKhanna commented Aug 18, 2021

Uh oh!

ChetanKhanna commented Aug 20, 2021

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

97littleleaf11 commented Apr 7, 2021 •

edited

Loading

97littleleaf11 May 9, 2021 •

edited

Loading