Maps, Unrolling, and Dynamic Frame Size #147

timfel · 2017-03-22T13:09:08Z

There is a lot of stuff that has accumulated in this branch. I'm adding comments to explain the different bits. Reviews appreciated.

…nch from a script file) - add codeclimate

…ment map transitions for Fixed pointer objects with some inline fields, assume all objects we pass by have a strategy and a class, simplify code and fix tests to honor that"" This reverts commit 78bc8f5.

…the latest strategy in a loop

This reverts commit d134857.

…ays get the latest strategy in a loop" This reverts commit c599ed5.

…rors this exposed in primitives

… mode

…POV for squeak is the squeak size

…ldn't happen after we're jitted, but we abort tracing in that case anyway

…ounds

timfel · 2017-03-22T13:11:06Z

rsqueakvm/constants.py

@@ -127,42 +127,42 @@
 SO_JIT_HOOK_RCVR = 59 # really selectorTrap

 constant_objects_in_special_object_table = {
-    "nil": (SO_NIL, "POINTERS"),


We use maps for W_FixedPointersObjects that have inline fields. These objects are prebuilt constants - we need to create them to be the right kind of empty PBCs.

timfel · 2017-03-22T13:12:57Z

rsqueakvm/interpreter.py

-                    w_class=self.getreceiverclass(s_context),
-                    blockmethod=self.getblockmethod(s_context),
-                    s_context=s_context)
+                if jump_back_pc == old_pc:


This allows us to manually unroll a variable amount of times. At least one time is useful in Squeak, because of the way loops are compiled by the Squeak compiler (see also the jittests for unrolling)

timfel · 2017-03-22T13:14:05Z

rsqueakvm/main.py

@@ -66,6 +66,8 @@ def _usage(argv):
          Execution:
            -r|--run <code>  - Code will be compiled and executed in
                               headless mode, result printed.
+            -rr <code>       - Code will be compiled and executed in


This is a bit of a hack: with the dynamic frame size calculation, we must not JIT a method that we don't know the frame size beforehand, so doits are never jitted. -rr just calls the passed code twice, so the second time we can jit.

I'm assuming this is for benchmark purposes? maybe add (used for benchmarks).

This is actually for the jittests, but I'll add a comment

timfel · 2017-03-22T13:15:32Z

rsqueakvm/interpreter.py

+                            # Do the interrupt-check at the end of a loop, don't
+                            # interrupt loops midway.
+                            self.jitted_check_for_interrupt(s_context)
+                        if not s_context.has_overflow_stack():


When we're in a loop where the initially allocated frame size was smaller than what we actually used in the loop, we'll have an overflow stack. The next time we enter this method the larger frame size will be allocated and no overflow stack will be needed, but we don't want to jit a loop that uses the overflow stack only for the one time we have it.

timfel · 2017-03-22T13:16:48Z

rsqueakvm/main.py

@@ -79,6 +81,9 @@ def _usage(argv):
                               synthetic high-prio Process.
            -u|--stop-ui     - Only with -m or -r. Try to stop UI-process at
                               startup. Can help benchmarking.
+            --run-file       - Run the .st file supplied as first argument. No


A bit of a hackish cmdline argument to support squeak-style invocation ./rsqueak --run-file Image run.st but side-stepping the entire image startup. This will not work with many files, but with some (like e.g. scripts that just start and then exit, or benchmarking scripts)

timfel · 2017-03-22T13:17:22Z

rsqueakvm/model/base.py

@@ -103,8 +103,8 @@ def invariant(self):

    def class_shadow(self, space):
        """Return internal representation of Squeak class."""
-        w_class = jit.promote(self.getclass(space))
-        return w_class.as_class_get_shadow(space)
+        w_class = self.getclass(space)


getclass promotes directly now

timfel · 2017-03-22T13:18:40Z

rsqueakvm/model/base.py

-        w_class = jit.promote(self.getclass(space))
-        return w_class.as_class_get_shadow(space)
+        w_class = self.getclass(space)
+        return jit.promote(w_class.as_class_get_shadow(space))


We want to promote this as early as possible, I think, it is used in sends

timfel · 2017-03-22T13:19:41Z

rsqueakvm/model/base.py

@@ -273,7 +262,6 @@ class W_AbstractObjectWithClassReference(W_AbstractObjectWithIdentityHash):
    """Objects with arbitrary class (ie not CompiledMethod, SmallInteger or
    Float)."""
    _attrs_ = ['w_class']
-    _immutable_fields_ = ['w_class?']


The DSA algorithm uses changeClass: on LargeInteger<->ByteArray, so this really isn't quasi-immutable in that loop. I'm hoping this doesn't hurt too much

timfel · 2017-03-22T13:20:39Z

rsqueakvm/model/compiled_methods.py

    @jit.elidable_promote()
-    def compute_frame_size(self):
+    def squeak_frame_size(self):


We now report the frame size as viewed by Squeak differently to the one that we discovered while running (stored in _frame_size).

timfel · 2017-03-22T13:21:09Z

rsqueakvm/model/compiled_methods.py

-        assert pc >= 0 and pc < len(self.bytes)
-        return self.bytes[pc]
+        try:
+            return self.bytes[pc]


I was getting crashes with some methods, I figured if we go out of bounds, rather than crashing, we can just do a return.

timfel · 2017-03-22T13:21:31Z

rsqueakvm/model/compiled_methods.py

+        if literals and len(literals) > 1:
+            w_symbol = literals[-2]
+            if isinstance(w_symbol, W_BytesObject):
+                self.lookup_selector = "".join(w_symbol.getbytes())


Works for at least some methods during fill in

timfel · 2017-03-22T13:23:29Z

rsqueakvm/model/compiled_methods.py

+    def frame_size(self):
+        return self._frame_size
+
+    def update_frame_size(self, size):


Frame sizes will only grow. This isn't perfect, because some loops might not require the whole frame, but this way we'll never end up with some assembler that tries to read more frame than what's there. I'm not certain this might be too conservative, though

timfel · 2017-03-22T13:24:24Z

rsqueakvm/model/pointers.py

 from rpython.rlib.rstrategies import rstrategies as rstrat


 class W_PointersObject(W_AbstractObjectWithIdentityHash):
    """Common object."""
    _attrs_ = ['strategy', '_storage']
-    # TODO -- is it viable to have these as pseudo-immutable?
-    # Measurably increases performance, since they do change rarely.
-    _immutable_attrs_ = ['strategy?', '_storage?']


Since we have map transitions and changeClass in benchmarks, these are not really quasi-immutable. I hope it doesn't hurt performance too much in other places.

timfel · 2017-03-22T13:29:17Z

rsqueakvm/model/pointers.py


-    def has_class(self):
-        return self.getclass(None) is not None
+    def elidable_strategy(self):


Since strategies are no longer quasi-immutable, we try to make the reads go away anyway if we have a constant self

timfel · 2017-03-22T13:32:12Z

rsqueakvm/model/pointers.py

+# getattrs)
+_NUMBER_OF_INT_FIELDS = 3
+_NUMBER_OF_INLINE_FIELDS = 2
+class W_FixedPointersObject(W_PointersObject):


Right now the number of inline fields here is truly random, i.e., determined by a fair dice roll. Not sure if we want to tweak that or have multiple FixedPointersObjects or what.

Maybe still add your reasoning for the initial values with a comment.

timfel · 2017-03-22T13:42:07Z

rsqueakvm/primitives/control.py

@@ -220,15 +220,16 @@ def func(interp, s_frame, argcount):
    w_selector = s_frame.pop()
    w_rcvr = s_frame.top() # rcvr is removed in _sendSelector
    return s_frame._sendSelector(
-        w_selector, argcount - 1, interp, w_rcvr,
+        jit.promote(w_selector), argcount - 1, interp, w_rcvr,


we promote the selector we're performing to elide the lookup

timfel · 2017-03-22T13:48:26Z

rsqueakvm/storage_contexts.py

@@ -311,8 +321,7 @@ def store_stackpointer(self, size):
            assert size >= 0, "trying to store negative stackpointer"
            self.pop_n(depth - size)
        else:
-            for i in range(depth, size):
-                self.push(self.space.w_nil)
+            self.store_stack_ptr(size)


There's already something on the stack (probably nil), it's fine to leak stuff also, there is not guarantee that the stack is nil-ed

timfel · 2017-03-22T13:49:16Z

rsqueakvm/storage_contexts.py

            stacksize = self.stacksize()  # no temps
        return stacksize

    def init_temps_and_stack(self):
        self = fresh_virtualizable(self)
        stacksize = self.full_stacksize()
-        self._temps_and_stack = [None] * stacksize
+        self._temps_and_stack = [self.space.w_nil] * stacksize


With the dynamic frame size, we're probably only allocating as much as necessary anyway, so this helps us again remove the guard in stack_get to check for None

timfel · 2017-03-22T13:49:49Z

rsqueakvm/storage_contexts.py

+        return False
+
+    @jit.unroll_safe
+    def update_stacksize(self, index0):


We cache on the compiled method how large the largest frame was that we needed for this method

timfel · 2017-03-22T13:53:51Z

rsqueakvm/test/jittest/test_modern.py

        """)

-    @py.test.mark.skipif("'Not ready'")


fniephaus

First pass, still have to look at the new Map classes...

fniephaus · 2017-03-22T13:40:22Z

rsqueakvm/interpreter.py

+    _immutable_fields_ = ["max_squeak_unroll_count", "squeak_unroll_trace_limit"]
+    def __init__(self):
+        self.max_squeak_unroll_count = 2
+        self.squeak_unroll_trace_limit = 32000


Could you add a comment to explain the two values?

fniephaus · 2017-03-22T13:42:00Z

rsqueakvm/main.py

@@ -66,6 +66,8 @@ def _usage(argv):
          Execution:
            -r|--run <code>  - Code will be compiled and executed in
                               headless mode, result printed.
+            -rr <code>       - Code will be compiled and executed in


I'm assuming this is for benchmark purposes? maybe add (used for benchmarks).

fniephaus · 2017-03-22T13:46:38Z

rsqueakvm/model/compiled_methods.py

-        j = 1
-        for i in self.bytes:
+        j = 0
+        while j < len(self.bytes):


why the while loop? for j in range(len(self.bytes)):?

fniephaus · 2017-03-22T13:47:47Z

rsqueakvm/model/compiled_methods.py

@@ -380,26 +408,32 @@ def as_string(self, markBytecode=0):

    def guess_containing_classname(self):
        w_class = self.compiled_in()
-        if w_class and w_class.has_space():
+        if w_class:


Maybe start explicitly with if w_class is None: and switch the return statements.

fniephaus · 2017-03-22T13:49:55Z

rsqueakvm/model/pointers.py

+# getattrs)
+_NUMBER_OF_INT_FIELDS = 3
+_NUMBER_OF_INLINE_FIELDS = 2
+class W_FixedPointersObject(W_PointersObject):


Maybe still add your reasoning for the initial values with a comment.

fniephaus · 2017-03-22T13:55:15Z

rsqueakvm/primitives/input_output.py

@@ -97,7 +97,7 @@ def func(interp, s_frame, argcount, w_method):

 @expose_primitive(SNAPSHOT, clean_stack=False, no_result=True)
 def func(interp, s_frame, argcount):
-    s_frame.pop_n(argcount)
+    s_frame.pop_n(argcount + 1)


Just checked, it's the only occurrence of pop_n(argcount) in rsqueakvm.*.

fniephaus · 2017-03-22T13:56:23Z

rsqueakvm/squeakimage.py

@@ -356,6 +356,11 @@ def get_bytes_of(self, chunk):
                bytes.append(chr((each >> 24) & 0xff))
        return bytes

+    def isfixed(self, g_object):
+        return (self.space.use_maps.is_set() and
+                g_object.format == 1 and


We don't have any consts for the different formats?

fniephaus · 2017-03-22T13:59:22Z

rsqueakvm/storage_contexts.py

-            self.extra_data = ExtraContextAttributes()
-        return self.extra_data
+        extra_data = self.extra_data_or_blockmethod
+        if extra_data is None or not isinstance(extra_data, ExtraContextAttributes):


put the easier case first: if isinstance(extra_data, ExtraContextAttributes): return...

fniephaus · 2017-03-22T14:01:35Z

rsqueakvm/storage_contexts.py

+        w_bm = self.extra_data_or_blockmethod
+        if isinstance(w_bm, ExtraContextAttributes):
+            w_bm = w_bm.blockmethod
+        if w_bm is None:


if isinstance(w_bm, W_CompiledMethod): return w_bm, otherwise return None. -> avoids assert.

fniephaus · 2017-03-22T14:02:55Z

rsqueakvm/test/jittest/test_modern.py

+        setfield_gc(ConstPtr(ptr83), i82, descr=<FieldS rsqueakvm.interpreter.Interpreter.inst_interrupt_check_counter 24>)
+        i85 = int_le(i82, 0)
+        guard_false(i85, descr=<Guard0x4a52360>)
+        jump(p0, p1, i2, p4, p6, p7, p9, p12, p14, i80, p22, p24, p26, p28, p30, p32, p34, p36, p38, p40, p42, p47, i82, descr=TargetToken(83088416))
        """)


can I haz more traces to read? 😄

coveralls · 2017-03-23T15:36:43Z

Coverage increased (+0.02%) to 80.329% when pulling bc0162c on tim/dynamic_frame_size into ec969cf on master.

…/dynamic_frame_size

…er for annotating rvmprof

coveralls · 2017-04-02T23:42:44Z

Coverage increased (+0.01%) to 80.319% when pulling f3ba427 on tim/dynamic_frame_size into ec969cf on master.

timfel added 30 commits January 31, 2017 15:24

make shell work after translation

a5fc296

- load files in the shell, allow -r to pass shell line (so we can lau…

c009380

…nch from a script file) - add codeclimate

Merge branch 'master' into tim/maps

8d8818c

Merge branch 'tim/guard_opt' into tim/maps

af7962b

Revert "Revert "split PointersObjects into Fixed and Non-Fixed, imple…

69edf19

…ment map transitions for Fixed pointer objects with some inline fields, assume all objects we pass by have a strategy and a class, simplify code and fix tests to honor that"" This reverts commit 78bc8f5.

Merge remote-tracking branch 'origin/master' into tim/maps

05516f3

Merge remote-tracking branch 'origin/master' into tim/maps

1ad1975

fix translation with immutability plugin

588e9e6

promote perform selectors

b604699

don't allocate _storage field if map doesn't need it

41bc04c

try to avoid some allocation overhead when switching strategies

165ada5

update the singleton nodes for MapStrategy, so that we'll always get …

c599ed5

…the latest strategy in a loop

use nan-tagging for nil in float and integer storages

d134857

Revert "use nan-tagging for nil in float and integer storages"

141029f

This reverts commit d134857.

Revert "update the singleton nodes for MapStrategy, so that we'll alw…

8c263aa

…ays get the latest strategy in a loop" This reverts commit c599ed5.

if w_self is constant, promote our strategy

96d3f22

if self is constant, call an elidable strategy accessor

a91b67f

compute the actually required frame size

547c288

fix tests and frame size calculation, as well as a couple of stack er…

89687c3

…rors this exposed in primitives

skip two bytes for Spur prim bytecode 139 as params

c8a58d2

fix tests and extend stack size computation

1f6bf27

switch to dynamic computation of frame size

360aaff

punt on untranslated for now

b96acde

use the discovered frame size and go through slow path in interpreted…

8a4591c

… mode

we need to have space at least for args and temps. the size from the …

b1c7eae

…POV for squeak is the squeak size

dynamically overflow into an 'overflow stack' array on the heap. shou…

d79c899

…ldn't happen after we're jitted, but we abort tracing in that case anyway

don't store the w_self_size, calculate it if required

5cd8292

merge blockmethod and extra_data into a single field

795f054

let's ignore all stores behind the stack pointer, even those out of b…

7d822df

…ounds

implement switches to disable or limit maps

d90f464

update jittests

93fdd55

timfel commented Mar 22, 2017

View reviewed changes

rsqueakvm/test/jittest/test_modern.py

""")

@py.test.mark.skipif("'Not ready'")

Copy link

Member Author

timfel Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😀

fniephaus reviewed Mar 22, 2017

View reviewed changes

timfel added 2 commits March 23, 2017 14:40

only interrupt on keyup

7526c30

address code review concerns

bc0162c

timfel added 4 commits March 28, 2017 10:30

Merge remote-tracking branch 'origin/tim/dynamic_frame_size' into tim…

a967a31

…/dynamic_frame_size

really only unroll once

f5f834a

make stack_frame no longer take a default argument, to make life easi…

c6b982c

…er for annotating rvmprof

update jittests

f3ba427

Maps, Unrolling, and Dynamic Frame Size #147

Are you sure you want to change the base?

Maps, Unrolling, and Dynamic Frame Size #147

Conversation

timfel commented Mar 22, 2017

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

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

fniephaus left a 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

coveralls commented Mar 23, 2017

coveralls commented Apr 2, 2017