[mypyc] Implement dict clear primitive #9724

vsakkas · 2020-11-15T11:31:51Z

Description

Implements dict clear primitive for improved performance.

Test Plan

Ensure that clearing a dict works as expected, write tests and calculate performance improvements.

Generated IR

The following script was used:

d = {'a': 1, 'b': 2, 'c': 3}
d.clear()
print(d)

Master branch:

def __top_level__():
    r0, r1 :: object
    r2 :: bit
    r3 :: str
    r4 :: object
    r5, r6, r7 :: str
    r8, r9, r10 :: object
    r11, r12 :: dict
    r13 :: str
    r14 :: int32
    r15 :: bit
    r16 :: dict
    r17 :: str
    r18 :: object
    r19 :: dict
    r20 :: str
    r21 :: object
    r22 :: dict
    r23 :: str
    r24 :: object
    r25 :: dict
    r26 :: object
    r27 :: str
    r28, r29 :: object
    r30 :: None
L0:
    r0 = builtins :: module
    r1 = load_address _Py_NoneStruct
    r2 = r0 != r1
    if r2 goto L3 else goto L1 :: bool
L1:
    r3 = load_global CPyStatic_unicode_0 :: static  ('builtins')
    r4 = PyImport_Import(r3)
    if is_error(r4) goto L13 (error at <module>:-1) else goto L2
L2:
    builtins = r4 :: module
    dec_ref r4
L3:
    r5 = load_global CPyStatic_unicode_1 :: static  ('a')
    r6 = load_global CPyStatic_unicode_2 :: static  ('b')
    r7 = load_global CPyStatic_unicode_3 :: static  ('c')
    r8 = box(short_int, 2)
    r9 = box(short_int, 4)
    r10 = box(short_int, 6)
    r11 = CPyDict_Build(3, r5, r8, r6, r9, r7, r10)
    dec_ref r8
    dec_ref r9
    dec_ref r10
    if is_error(r11) goto L13 (error at <module>:1) else goto L4
L4:
    r12 = program.globals :: static
    r13 = load_global CPyStatic_unicode_4 :: static  ('d')
    r14 = CPyDict_SetItem(r12, r13, r11)
    dec_ref r11
    r15 = r14 >= 0 :: signed
    if not r15 goto L13 (error at <module>:1) else goto L5 :: bool
L5:
    r16 = program.globals :: static
    r17 = load_global CPyStatic_unicode_4 :: static  ('d')
    r18 = CPyDict_GetItem(r16, r17)
    if is_error(r18) goto L13 (error at <module>:2) else goto L6
L6:
    r19 = cast(dict, r18)
    if is_error(r19) goto L13 (error at <module>:2) else goto L7
L7:
    r20 = load_global CPyStatic_unicode_5 :: static  ('clear')
    r21 = CPyObject_CallMethodObjArgs(r19, r20, 0)
    dec_ref r19
    if is_error(r21) goto L13 (error at <module>:2) else goto L14
L8:
    r22 = program.globals :: static
    r23 = load_global CPyStatic_unicode_4 :: static  ('d')
    r24 = CPyDict_GetItem(r22, r23)
    if is_error(r24) goto L13 (error at <module>:3) else goto L9
L9:
    r25 = cast(dict, r24)
    if is_error(r25) goto L13 (error at <module>:3) else goto L10
L10:
    r26 = builtins :: module
    r27 = load_global CPyStatic_unicode_6 :: static  ('print')
    r28 = CPyObject_GetAttr(r26, r27)
    if is_error(r28) goto L15 (error at <module>:3) else goto L11
L11:
    r29 = PyObject_CallFunctionObjArgs(r28, r25, 0)
    dec_ref r28
    dec_ref r25
    if is_error(r29) goto L13 (error at <module>:3) else goto L16
L12:
    return 1
L13:
    r30 = <error> :: None
    return r30
L14:
    dec_ref r21
    goto L8
L15:
    dec_ref r25
    goto L13
L16:
    dec_ref r29
    goto L12

PR:

def __top_level__():
    r0, r1 :: object
    r2 :: bit
    r3 :: str
    r4 :: object
    r5, r6, r7 :: str
    r8, r9, r10 :: object
    r11, r12 :: dict
    r13 :: str
    r14 :: int32
    r15 :: bit
    r16 :: dict
    r17 :: str
    r18 :: object
    r19, r20 :: dict
    r21 :: str
    r22 :: object
    r23 :: dict
    r24 :: object
    r25 :: str
    r26, r27 :: object
    r28 :: None
L0:
    r0 = builtins :: module
    r1 = load_address _Py_NoneStruct
    r2 = r0 != r1
    if r2 goto L3 else goto L1 :: bool
L1:
    r3 = load_global CPyStatic_unicode_0 :: static  ('builtins')
    r4 = PyImport_Import(r3)
    if is_error(r4) goto L12 (error at <module>:-1) else goto L2
L2:
    builtins = r4 :: module
    dec_ref r4
L3:
    r5 = load_global CPyStatic_unicode_1 :: static  ('a')
    r6 = load_global CPyStatic_unicode_2 :: static  ('b')
    r7 = load_global CPyStatic_unicode_3 :: static  ('c')
    r8 = box(short_int, 2)
    r9 = box(short_int, 4)
    r10 = box(short_int, 6)
    r11 = CPyDict_Build(3, r5, r8, r6, r9, r7, r10)
    dec_ref r8
    dec_ref r9
    dec_ref r10
    if is_error(r11) goto L12 (error at <module>:1) else goto L4
L4:
    r12 = program.globals :: static
    r13 = load_global CPyStatic_unicode_4 :: static  ('d')
    r14 = CPyDict_SetItem(r12, r13, r11)
    dec_ref r11
    r15 = r14 >= 0 :: signed
    if not r15 goto L12 (error at <module>:1) else goto L5 :: bool
L5:
    r16 = program.globals :: static
    r17 = load_global CPyStatic_unicode_4 :: static  ('d')
    r18 = CPyDict_GetItem(r16, r17)
    if is_error(r18) goto L12 (error at <module>:2) else goto L6
L6:
    r19 = cast(dict, r18)
    if is_error(r19) goto L12 (error at <module>:2) else goto L7
L7:
    CPyDict_Clear(r19)
    dec_ref r19
    r20 = program.globals :: static
    r21 = load_global CPyStatic_unicode_4 :: static  ('d')
    r22 = CPyDict_GetItem(r20, r21)
    if is_error(r22) goto L12 (error at <module>:3) else goto L8
L8:
    r23 = cast(dict, r22)
    if is_error(r23) goto L12 (error at <module>:3) else goto L9
L9:
    r24 = builtins :: module
    r25 = load_global CPyStatic_unicode_5 :: static  ('print')
    r26 = CPyObject_GetAttr(r24, r25)
    if is_error(r26) goto L13 (error at <module>:3) else goto L10
L10:
    r27 = PyObject_CallFunctionObjArgs(r26, r23, 0)
    dec_ref r26
    dec_ref r23
    if is_error(r27) goto L12 (error at <module>:3) else goto L14
L11:
    return 1
L12:
    r28 = <error> :: None
    return r28
L13:
    dec_ref r23
    goto L12
L14:
    dec_ref r27
    goto L11

Performance

The following script was used:

for i in range(1000000):
    d = {'a': 1, 'b': 2, 'c': 3}
    d.clear()

Master branch: 0.97x (In this test, I mostly noticed a slowdown compared to running the script as is with Python 3.8)
PR: 1.14x speedup

JukkaL · 2020-11-16T13:26:38Z

The test failures are probably caused with a problem with dict subclasses, such as OrderedDict. You'll need to check if the type is exactly dict before using PyDict_Clear (using PyDict_CheckExact). If it's an instance of a subclass, you'll need to call the method using a generic API function (e.g. PyObject_CallMethodNoArgs) so that an overridden method will be called. You can create a C helper function such as CPyDict_Clear that does this.

vsakkas · 2020-11-16T13:32:06Z

Thank you @JukkaL for the clarification. This is the first time I'm working with the mypyc codebase, so I'm still trying to get familiar with how it works internally.

I'll have a look at it (as well as the copy primitive, which has similar issues) and update both my PRs as soon as possible.

vsakkas · 2020-11-18T10:04:17Z

Test are finally passing!

vsakkas · 2020-11-18T19:52:27Z

So, I just discovered that this repo exists https://github.com/mypyc/mypyc-benchmarks so I'll post the results from that as well:

Master:

running dict_clear
..........
interpreted: 0.314835s (avg of 5 iterations; stdev 1.2%)
compiled:    0.100214s (avg of 5 iterations; stdev 1.1%)

compiled is 3.142x faster

PR:

running dict_clear
..........
interpreted: 0.316260s (avg of 5 iterations; stdev 1.1%)
compiled:    0.082226s (avg of 5 iterations; stdev 0.3%)

compiled is 3.846x faster

JukkaL

Just a few minor comments, otherwise looks good! I like the performance improvement.

JukkaL · 2020-11-19T12:37:23Z

mypyc/test-data/run-dicts.test

+
+d = {'a': 1, 'b': 2}
+d.clear()
+assert d == {}


It would be nice to also test this with a DefaultDict instance (still using Dict[...] as the declared type) to test the other code path.

See my most recent comments in #9721 -- they also apply here.

At least the lines that call clear() need to be moved away from driver.py, since this file doesn't get compiled.

JukkaL · 2020-11-19T12:38:04Z

mypyc/primitives/dict_ops.py

+    arg_types=[dict_rprimitive],
+    return_type=void_rtype,
+    c_function_name='CPyDict_Clear',
+    error_kind=ERR_NEVER)


Use ERR_MAGIC here since an overridden method might raise an exception.

I'm a little confused about this one. If I change the error_kind to ERR_MAGIC, I get this error:

assert not op.is_void, "void op generating errors?"

Due to the fact that the return type is void_rtype. However, what should I change this to? CPyDict_Clear uses internally PyDict_Clear which has a void return type itself.

Ah one option is t to use bit_rprimitive as the return type (char in the C function) and ERR_FALSE as the error kind. Then unconditionally return 1 when there can be no error. 0 signnals an exception being raised.

@JukkaL I have added case testDictMethods to be used for testing copy, clear and any other dict methods. Let me know if it looks good to you.

JukkaL

Looks good now! Thanks for the updates.

Here are a few updates/fixes after a read through the mypyc docs. For the corresponding primitive method updates, see: * str.replace: #10088 * dict.copy: #9721 * dict.clear: #9724

Implement dict clear primitive

72bb5c7

vsakkas force-pushed the dict-clear branch 4 times, most recently from 34f6041 to f9e50f9 Compare November 17, 2020 22:43

vsakkas marked this pull request as draft November 18, 2020 08:49

Implement dict clear C helper function

c728478

vsakkas force-pushed the dict-clear branch from f9e50f9 to c728478 Compare November 18, 2020 09:15

vsakkas marked this pull request as ready for review November 18, 2020 10:03

JukkaL reviewed Nov 19, 2020

View reviewed changes

vsakkas added 2 commits November 20, 2020 22:28

Update dict clear tests

72f73d5

Update dict clear error kind

db962b8

vsakkas force-pushed the dict-clear branch from baed67d to db962b8 Compare November 20, 2020 20:57

vsakkas added 3 commits November 22, 2020 15:34

Update dict clear return and error types

195e4bb

Update dict clear tests

e7db60e

Update dict clear tests

31e0d58

JukkaL approved these changes Dec 2, 2020

View reviewed changes

JukkaL merged commit 98eee40 into python:master Dec 2, 2020

thomasjohns mentioned this pull request Mar 8, 2021

[mypyc] Small documentation updates #10184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mypyc] Implement dict clear primitive #9724

[mypyc] Implement dict clear primitive #9724

vsakkas commented Nov 15, 2020 •

edited

Loading

JukkaL commented Nov 16, 2020

vsakkas commented Nov 16, 2020

vsakkas commented Nov 18, 2020

vsakkas commented Nov 18, 2020

JukkaL left a comment

JukkaL Nov 19, 2020

JukkaL Nov 22, 2020

JukkaL Nov 27, 2020

JukkaL Nov 19, 2020

vsakkas Nov 20, 2020

JukkaL Nov 22, 2020

vsakkas Nov 29, 2020

JukkaL left a comment

[mypyc] Implement dict clear primitive #9724

[mypyc] Implement dict clear primitive #9724

Conversation

vsakkas commented Nov 15, 2020 • edited Loading

Description

Test Plan

Generated IR

Performance

JukkaL commented Nov 16, 2020

vsakkas commented Nov 16, 2020

vsakkas commented Nov 18, 2020

vsakkas commented Nov 18, 2020

JukkaL left a comment

Choose a reason for hiding this comment

JukkaL Nov 19, 2020

Choose a reason for hiding this comment

JukkaL Nov 22, 2020

Choose a reason for hiding this comment

JukkaL Nov 27, 2020

Choose a reason for hiding this comment

JukkaL Nov 19, 2020

Choose a reason for hiding this comment

vsakkas Nov 20, 2020

Choose a reason for hiding this comment

JukkaL Nov 22, 2020

Choose a reason for hiding this comment

vsakkas Nov 29, 2020

Choose a reason for hiding this comment

JukkaL left a comment

Choose a reason for hiding this comment

vsakkas commented Nov 15, 2020 •

edited

Loading