Fixed interpreted program. #2

jaheba · 2017-03-23T15:59:18Z

Not cleaned up, but seems to work :)

ltratt · 2017-03-23T17:33:45Z

Once Jasper's tidies this up a bit, it should be ready for review by @vext01 and me.

vext01 · 2017-03-24T09:53:02Z

I don't have any context. Is this is just an interpreter written in rust? What's GreenMT? Is it a re-naming of grass?

jaheba · 2017-03-24T09:58:09Z

@vext01 Let me add a README. In short it's a tracer.

ltratt · 2017-03-24T22:31:02Z

Jasper, are you saying this is now ready for review or ...?

jaheba · 2017-03-24T22:31:48Z

Well, yes.

ltratt · 2017-03-25T10:38:55Z

Before Edd and I do a review, can we get the formatting within the project consistent? I don't mind too much what it is, so long as it's consistent (e.g. blank lines between structs/functions). When it's inconsistent, it's a distraction that prevents reviewers concentrating on the important stuff.

jaheba · 2017-03-25T11:19:40Z

Will do, let me see if rustfmt has some option for that. What style guide are you using for lrtable?

ltratt · 2017-03-25T11:20:23Z

I use "Laurie's patented style". At this point, it doesn't matter too much provided you're a) not doing something no-one else has ever done b) it's consistent within the project.

vext01

Lots of small comments.

I didn't see the call inlining code in my review. Can you point it out?

vext01 · 2017-03-27T08:50:49Z

README.md

@@ -0,0 +1,24 @@
+# daly
+Simple VM for a Dyon--subset


Link to dyon website? I've never heard of dyon, so it's likely others haven't either.

Is it just a VM, or a VM and a tracer. A bullet point below leas me to think the latter.

vext01 · 2017-03-27T08:53:03Z

README.md

+### Optimisations
+
+**Inlining**
+Inlining of function calls is performed. Necessary steps for deoptimisation can be found in `tracerunner::Runner::recover`.


Deoptimisation for inlining would be pulling an inlined function back out into its own unit with an activation record etc? Why would you need to do this? Are you inlining specialised versions of functions perhaps?

Put another way, why would you want to "un-inline" a call?

Maybe this is a great idea for some reason, but it's not a conventional approach (AFAIK).

Let me give you a (somewhat artificial) example. Imagine you've got a function f which inspects the stack. If you inline f, you then have to deopt so that it can inspect the stack; you'd be better off not actually inlining it but you might only realise that after you've inlined it.

That said, in general, you're right. But I think the point Jasper is making is not about uninlining, so much as it's about "we called f, a guard failed, so we have to blackhole".

vext01 · 2017-03-27T08:54:34Z

src/conversions.rs

+}
+
+
+// impl<'a> From<&'a Instruction> for TraceInstruction {


Commented code.

vext01 · 2017-03-27T08:56:02Z

src/conversions.rs

+        use TraceInstruction as TI;
+
+        match instr {
+            I::Add => TI::Add,


Is the spacing here implying some kind of grouping? If so, add a comment, if not kill the whitespace?

vext01 · 2017-03-27T08:56:25Z

src/main.rs

@@ -1,19 +1,61 @@

-#[macro_use] extern crate maplit;
+// btreemap! macro


commented code

this is actually a comment, not commented code

maplit enables using the btreemap! macro

hah, sorry.

vext01 · 2017-03-27T09:31:18Z

src/main.rs


-                Clone => (),
+                // XXX


Expand on this XXX?

vext01 · 2017-03-27T09:35:32Z

src/tracerunner.rs

+
+pub struct Runner<'a, 'b: 'a> {
+    pub trace: &'a [TraceInstruction],
+    pub stack: Vec<Value>,


Should the stack be a fixed size array? I'm not sure.

It certainly could have a reserved capacity at least.

vext01 · 2017-03-27T09:37:41Z

src/tracerunner.rs

+
+            info!("TEXEC: {:?}", instr);
+
+            match *instr {


So the traces do the same as the interpreter for now? Let's add a comment?

vext01 · 2017-03-27T09:38:21Z

src/tracerunner.rs

+        }
+    }
+
+    // XXX: return None guard succeeds


Do you want to fix that here?

This line can actually be removed. guard used to return Option<usize>.

vext01 · 2017-03-27T09:42:26Z

src/tracerunner.rs

+    /// the following things have to be recovered
+    /// * stack-frames (call-frames)
+    /// * value stack (essentially bool which caused guard to fail)
+    fn recover(&mut self, guard: &Guard) {


This is blackholing. I was expecting this part to be much harder. Does this implementation have limitations?

Hmm, I guess it gets harder if one adds more optimisations -- each added optimisation possible requires some de-optimisation steps. I wonder what had to changed, if we allow references to stack variables.

I don't really know much about black-holing, so you are kinda on your own here ;)

vext01 · 2017-03-27T13:26:43Z

Added more comments.

Can you point out where calls are being inlined. I can't see where this happens.

jaheba · 2017-03-27T13:35:45Z

Inlining actually happens in multiple places.

Basically inlining just means, that stackframes of called functions are integrated into the one and only frame. Thus load and store instructions have to modified to account for the new local positions. Similarly arguments are stored into locals.

When calling a function during tracing, we have to reserve some space for the local variables and store recovery information for guards.

I think I should add some comments about that in the code.

vext01 · 2017-03-27T13:43:04Z

Yes, it needs to be clearer. Usually you'd let tracing follow the function call (instead of inserting a call into the trace) and then let the trace optimiser remove the (now redundant) calling convention code (like you say, the locals of the callee may as well be locals of the caller). But I've never seem a VM deoptimise (un-inline) a function which a comment alludes to in the README.

ltratt · 2017-03-27T14:57:45Z

@jaheba So I suggest you address Edd's comments (however long that takes), and then I'll have a go. Sound OK?

jaheba · 2017-03-30T12:52:49Z

I'm on it!

jaheba · 2017-04-04T15:10:39Z

@ltratt I think the code is now in a cleaner state. Maybe have a quick look, what I should address before you do the full review?

ltratt · 2017-04-05T09:02:07Z

Oops, I think you accidentally committed your Cargo.lock file?

jaheba · 2017-04-05T09:06:41Z

Not that accidently. rust-lang/cargo#315 http://doc.crates.io/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries Just gollowing the rules.

…

On Wed, 5 Apr 2017 at 10:02, Laurence Tratt ***@***.***> wrote: Oops, I think you accidentally committed your Cargo.lock file? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIKZNs3K-i-Hp-AoLXLz4OxK071BDR5ks5rs1iQgaJpZM4Mm-D3> .

ltratt · 2017-04-05T09:09:20Z

Yuck :( I don't think this is very useful for early-stages programs, as it just clogs up the diffs. But I'm not going to argue against it.

ltratt · 2017-04-05T09:19:36Z

src/main.rs

+        let mut locals = TraceDataAllocator::new();
+        locals.alloc(instr.func.args_count + instr.func.locals_count);
+
+        let mut next = instr.clone();


Why do we have to clone instructions all the time? Is this a restriction imposed by the borrow checker or ... ?

ltratt · 2017-04-05T09:31:58Z

src/main.rs

@@ -192,167 +185,190 @@ impl<'a> Interpreter<'a> {
                ArrayGet => self.do_array_get(),

                Call(ref target) => {
-                    let new_func = self.module.funcs.get(target).unwrap();
-                    let mut frame = CallFrame::for_fn(new_func, (func, pc));
+                    let new_func = self.module.funcs[target].clone();


Eek, cloning the function is pretty heavyweight isn't it, given that we don't appear to be mutating it?

ltratt · 2017-04-05T09:32:24Z

src/main.rs

                    }

+                    call_tree = call_tree.push(FrameInfo {
+                        func: new_func.clone(),


Isn't this cloning the clone?

I've removed the first clone.

ltratt · 2017-04-05T10:49:43Z

src/tracerunner.rs

+    ///       Thus, we have to reconstruct all missing callframes, before the
+    ///       the interpreter can gain back control.
+    ///       Second, we also have to consider the frame where the loop resides
+    ///       in, since state might has also has changed there.


"has also" -> "have also"

ltratt · 2017-04-05T10:51:52Z

src/tracerunner.rs

+        let frames = guard.frame.walk().collect::<Vec<_>>();
+
+        // why .rev()? We rebuild the stack of frames, so we have to start with
+        // the bottom most.


Don't you mean the topmost frame?

Is topmost the latest item which was pushed onto the stack?

Typically "topmost" means "frame at the start of the stack" (and so "bottommost" means "frame at the end of the stack"). In retrospect, I'm not sure if my comment was correct or not as to the order in which you're doing things.

Maybe it's better to talk about a concrete example.

s = Stack() s.push(1) s.push(2) s.push(3)

In my view, 1 is on the bottom of the stack, so it is the bottom-most value. The top of the stack is the last value added, in this case 3. What I want to say in the comment is that we have to start with 1 to rebuild the stack.

Ah, maybe we should use "most" and "least" "recent" for clarity.

ltratt · 2017-04-05T10:52:22Z

src/tracerunner.rs

+    fn recover(&mut self, guard: &Guard) {
+        // remove the last callframe of the Interpreter
+        // it gets replaced with our updated version
+        self.interp.frames.pop().unwrap();


I'm a bit surprised that we can discard the current frame entirely. I assumed (maybe wrongly!) that it would have locals (etc.) which would need to be copied in the to-be-created frames?

The locals of the frame are copied into the trace-locals in Runner::new. The locals are restored in the first iteration of the following loop.

Yes, but won't the locals in the frame have been (potentially) changed, meaning they no longer matched those stored in the Runner?

As far as I know, only mutable references or a concurrent actor could have changed something in the original frame. Every other modification happens within the trace's locals.

ltratt · 2017-04-05T10:54:19Z

src/tracerunner.rs

+
+            // 2. fill it up with locals
+            for idx in 0..frame.locals.len() {
+                frame.locals[idx] = self.locals[frame_info.offset + idx].clone();


I'm not quite sure how we can get the right version of all the locals from other frames? Won't some have been updated?

I'm not sure I understand your question. Unless we have concurrent execution, I believe we should be fine.

This is related to the comment about "can we pop the frame", so let's consider the topic in that conversation.

ltratt · 2017-04-05T10:54:28Z

src/traits.rs

@@ -0,0 +1,29 @@
+
+pub mod vec {
+


Blank line.

ltratt · 2017-04-05T10:55:51Z

OK, I've made various comments. I think we could really do with some more tests, particularly to really test blackholing. I'm also curious if we could reduce the cloneing, which looks like a bit of a performance hit (not that we care about performance at the moment, as such, but it might suggest that we haven't got quite the right architecture, which we do care about).

jaheba · 2017-04-05T10:56:13Z

Yuck :( I don't think this is very useful for early-stages programs, as it just clogs up the diffs. But I'm not going to argue against it.

Nothing I feel strongly about. Should I just remove the Cargo.lock again?

ltratt · 2017-04-05T10:56:49Z

Up to you. I don't feel strongly about it, I guess, provided that it doesn't get updated every 5 minutes.

jaheba · 2017-04-05T10:59:30Z

Alright, then I'll let it in and we see how it goes.

jaheba · 2017-04-05T11:02:55Z

I'm also curious if we could reduce the cloneing, which looks like a bit of a performance hit (not that we care about performance at the moment, as such, but it might suggest that we haven't got quite the right architecture, which we do care about).

I think, some clones are just unnecessary and others could be avoided. Also, since I've opted for using Rcs all over the place, it makes cloneing mandatory.

ltratt · 2017-04-05T11:04:50Z

Ah, so some of the clones are Rcs. I didn't realise that (damn type inference!).

jaheba · 2017-04-05T11:06:09Z

One thing I'm a bit unsure about is how to handle new traits, which I implement for common structs. In my case, I implement some methods on top of Vec. To make this more clear I've decided to create a module called traits, where I put the implementation (use traits::vec::ConvertingStack).

@ltratt @vext01 thoughts?

ltratt · 2017-04-05T11:09:41Z

I wasn't totally certain if all the From conversions and so on were a help or a hindrance, to be honest...

ltratt · 2017-04-06T11:10:27Z

OK let's squash some of the stuff away, and then I can merge.

jaheba · 2017-04-06T13:32:06Z

I've squashed some commits together.

ltratt · 2017-04-06T13:33:46Z

Can we give the "cleanup" commit a slightly more meaningful title?

Edd unavailable due to travelling.

Fixed interpreted program.

2cb3eb4

jaheba requested a review from ltratt March 23, 2017 17:34

jaheba force-pushed the deopt branch from 0dcc26e to d74f9df Compare March 24, 2017 09:58

jaheba assigned vext01 Mar 24, 2017

jaheba requested a review from vext01 March 24, 2017 10:18

vext01 previously requested changes Mar 27, 2017

View reviewed changes

ltratt reviewed Apr 5, 2017

View reviewed changes

src/traits.rs Outdated

@@ -0,0 +1,29 @@

pub mod vec {

Copy link

Contributor

ltratt Apr 5, 2017

Choose a reason for hiding this comment

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

Blank line.

jaheba force-pushed the deopt branch 2 times, most recently from d7548f3 to 89b1e17 Compare April 6, 2017 13:30

ltratt approved these changes Apr 6, 2017

View reviewed changes

jaheba added 3 commits April 6, 2017 14:34

Added From conversions to stack push/pop. Addressed comments.

b159c64

Simplified trace-locals "allocation".

5cab23d

Refactoring / Addressing comments.

6835921

jaheba force-pushed the deopt branch from 89b1e17 to 6835921 Compare April 6, 2017 13:36

ltratt approved these changes Apr 6, 2017

View reviewed changes

ltratt self-assigned this Apr 6, 2017

ltratt merged commit b27ac9d into master Apr 6, 2017

ltratt deleted the deopt branch April 6, 2017 13:41

		@@ -1,19 +1,61 @@

		#[macro_use] extern crate maplit;
		// btreemap! macro

Fixed interpreted program. #2

Fixed interpreted program. #2

Conversation

jaheba commented Mar 23, 2017

ltratt commented Mar 23, 2017

vext01 commented Mar 24, 2017

jaheba commented Mar 24, 2017

ltratt commented Mar 24, 2017

jaheba commented Mar 24, 2017

ltratt commented Mar 25, 2017

jaheba commented Mar 25, 2017

ltratt commented Mar 25, 2017

vext01 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

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

vext01 commented Mar 27, 2017

jaheba commented Mar 27, 2017

vext01 commented Mar 27, 2017

ltratt commented Mar 27, 2017

jaheba commented Mar 30, 2017

jaheba commented Apr 4, 2017

ltratt commented Apr 5, 2017

jaheba commented Apr 5, 2017 via email

ltratt commented Apr 5, 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

ltratt commented Apr 5, 2017

jaheba commented Apr 5, 2017

ltratt commented Apr 5, 2017

jaheba commented Apr 5, 2017

jaheba commented Apr 5, 2017

ltratt commented Apr 5, 2017

jaheba commented Apr 5, 2017

ltratt commented Apr 5, 2017

ltratt commented Apr 6, 2017

jaheba commented Apr 6, 2017

ltratt commented Apr 6, 2017