Use an array instead of a dict for local variables #2355

coolreader18 · 2020-12-02T19:05:23Z

Benchmark #1: ./rustpython-norm benchmarks/pystone.py 50000
  Time (mean ± σ):      4.727 s ±  0.266 s    [User: 4.686 s, System: 0.015 s]
  Range (min … max):    4.416 s …  5.126 s    10 runs

Benchmark #2: ./rustpython-opt benchmarks/pystone.py 50000
  Time (mean ± σ):      3.875 s ±  0.211 s    [User: 3.847 s, System: 0.007 s]
  Range (min … max):    3.601 s …  4.261 s    10 runs

Summary
  './rustpython-opt benchmarks/pystone.py 50000' ran
    1.22 ± 0.10 times faster than './rustpython-norm benchmarks/pystone.py 50000'

Didn't get as much of a speedup as I was hoping for, but this should improve memory usage. Specifically, I was running into a problem where closures defined in a function would never get deallocated so it would really slow down after like 50 iterations. I think this should fix that, since now it's just a cell. It entire scope being kept around, including the function itself (locals()['bar'].outer_locals is locals()), making a reference cycle that would never dealloc.

def foo(big_object):
    def bar():
        use(big_object)
    # before, just because of bar, big_object would never be deallocated

for i in range(1000):
    inp = "x" * 10000
    foo(inp)

I wasn't able to get it to cause a slowdown on my machine with the master branch version of the binary, but the real code that this was happening with runs in wasm, so I assume that has something to do with it.

Edit: Oh, duh, here:

def foo(arg):
    def bar():
        pass

class Foo:
    def __del__(self):
        print("deleting")

for i in range(1000):
    foo(Foo())

$ benchmarks/rustpython-norm testt.py 
$ benchmarks/rustpython-opt testt.py 
deleting
deleting
deleting
deleting
deleting
deleting
deleting
deleting
deleting
<snip>

youknowone · 2020-12-04T15:35:44Z

bytecode/src/bytecode.rs

@@ -668,17 +669,24 @@ impl<C: Constant> CodeObject<C> {
    }

    pub fn map_bag<Bag: ConstantBag>(self, bag: &Bag) -> CodeObject<Bag::Constant> {
+        let map_names = |names: Box<[C::Name]>| {
+            names
+                .into_vec()


🤔 rust-lang/rust#59878

compiler/src/compile.rs

youknowone · 2020-12-04T15:57:45Z

compiler/src/symboltable.rs

+        }
+    }
+    impl<T> StackStack<T> {
+        pub fn append<F, R>(&mut self, x: &mut T, f: F) -> R


This function itself seems like not appending something but the given f do that, right?

Yeah, I wasn't totally sure on the naming, cause this is kinda a weird data structure. It calls f() in the context of x being appended to the stack; f can append more once inside that context but append is the one that actually calls push and pop. 🤷

I am not sure how to call this but at lease append gives wrong sense of the role of the method. Giving something weird or long name might be better than append.

Renamed it to with_append and added a doc comment.

derive/src/pyclass.rs

vm/src/builtins/dict.rs

youknowone · 2020-12-04T16:17:13Z

vm/src/dictdatatype.rs

@@ -275,6 +284,20 @@ impl<T: Clone> Dict<T> {
        Ok(ret)
    }

+    pub fn get2<K: DictKey>(


get2 doesn't look like the best name for this.
This is a sort of chained get and only used by frame.rs. I think these things implies this is not an expected feature as a method.
How about exposing _get_inner as get_raw(or somewhat common term in hash map libraries) as its low-level api and making a helper funciton in frame.rs?

I renamed it get_chain; I could make it pub(crate) if you want, but I think it's best to keep the dict implementation in the dict implementation- builtins::dict already has all the logic regarding exact instance vs subclass checking, so I think it makes sense to keep it there rather than move it to frame.

youknowone · 2020-12-04T16:37:45Z

vm/src/builtins/function.rs

-        let scope = if self.code.flags.contains(bytecode::CodeFlags::NEW_LOCALS) {
-            scope.new_child_scope(&vm.ctx)
+        let locals = if self.code.flags.contains(bytecode::CodeFlags::NEW_LOCALS) {
+            vm.ctx.new_dict()


what if locals is given but the flag is NEW_LOCALS? locals looks like not used in this case.
Is this possible scenario?

I think the cpython code for this basically ignores the passed locals if that happens:

/* Most functions have CO_NEWLOCALS and CO_OPTIMIZED set. */ if ((code->co_flags & (CO_NEWLOCALS | CO_OPTIMIZED)) == (CO_NEWLOCALS | CO_OPTIMIZED)) ; /* f_locals = NULL; will be set by PyFrame_FastToLocals() */ else if (code->co_flags & CO_NEWLOCALS) { locals = PyDict_New(); if (locals == NULL) { Py_DECREF(f); return NULL; } f->f_locals = locals; } else { if (locals == NULL) locals = globals; Py_INCREF(locals); f->f_locals = locals; }

youknowone · 2020-12-04T16:52:55Z

vm/src/dictdatatype.rs

+        if let Some(x) = self._get_inner(vm, key, hash)? {
+            Ok(Some(x))
+        } else {
+            other._get_inner(vm, key, hash)
+        }


Suggested change

if let Some(x) = self._get_inner(vm, key, hash)? {

Ok(Some(x))

} else {

other._get_inner(vm, key, hash)

}

self._get_inner(vm, key, hash).or_else(|| other._get_inner(vm, key, hash))

get_inner returns Result<Option>, I think if we wanted to use combinators it would be a bunch of transpose() calls so I figured this would be the cleanest way to do it.

Oh, yep. I missed Result. But not sure about transpose() because Result is unwrappable.

How about this?

Ok(self._get_inner(vm, key, hash)?.or_else(|| other._get_inner(vm, key, hash)?))

The ? inside the or_else wouldn't return from the outer function, it would return from the closure

vm/src/frame.rs

vm/src/stdlib/symtable.rs

youknowone reviewed Dec 4, 2020

View reviewed changes

coolreader18 force-pushed the coolreader18/funcvars branch from ed81105 to 217f2c3 Compare December 4, 2020 20:54

youknowone approved these changes Dec 5, 2020

View reviewed changes

coolreader18 force-pushed the coolreader18/funcvars branch from 217f2c3 to 04d50da Compare December 5, 2020 22:32

coolreader18 added 14 commits December 5, 2020 16:36

Add support for deleter on getsets

2c29a33

Add the cell class

6c20c3e

Add PyTupleTyped

3d44744

Move compiler snapshots

479610f

Fast locals part 1

97029af

Fast locals part 2

92b8e59

Fast locals part 3

7c58f4a

Try some mini-optimizations

cd87f6d

Use a boxed slice for codeobject fields

22e76f1

Update jit to use new instructions

6c52c5c

Update cranelift dependencies

0fddb04

Fix snippet test+wasm errors

150d5ae

Inline some hot functions

2bdbe6b

Apply review suggestions

34bb5f0

coolreader18 force-pushed the coolreader18/funcvars branch from 04d50da to 34bb5f0 Compare December 5, 2020 22:37

coolreader18 merged commit 51c2ec5 into master Dec 5, 2020

coolreader18 deleted the coolreader18/funcvars branch December 5, 2020 23:29

coolreader18 mentioned this pull request Dec 21, 2020

code objects: missing attributes #1838

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use an array instead of a dict for local variables #2355

Use an array instead of a dict for local variables #2355

coolreader18 commented Dec 2, 2020 •

edited

Loading

youknowone Dec 4, 2020

youknowone Dec 4, 2020

coolreader18 Dec 4, 2020

youknowone Dec 5, 2020

coolreader18 Dec 5, 2020

youknowone Dec 4, 2020

coolreader18 Dec 4, 2020

youknowone Dec 4, 2020

coolreader18 Dec 4, 2020

youknowone Dec 4, 2020

coolreader18 Dec 4, 2020

youknowone Dec 5, 2020

coolreader18 Dec 5, 2020 •

edited

Loading

Use an array instead of a dict for local variables #2355

Use an array instead of a dict for local variables #2355

Conversation

coolreader18 commented Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolreader18 Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

coolreader18 commented Dec 2, 2020 •

edited

Loading

coolreader18 Dec 5, 2020 •

edited

Loading