Global variables are now loaded from _ENV.

[rbartlensky]

In the Lua specification, the global variables are loaded from a global
table called _ENV. Programs can overwrite this variable as well, and
can read and write from/to it.
There are a few changes:

• The compiler:
  • assumes that _ENV is always stored in register 0
  • replaces all global variable writing, and reading with GetAttr, and
    SetAttr instructions
• The vm:
  • always creates a LuaTable and stores it in register 0, so that globals
    can be easily accessed.
  • can now set and get attributes of LuaTables

[ltratt]

Can you benchmark a couple of programs that do simple global read/writes before/after this PR?

[rbartlensky]

Can you benchmark a couple of programs that do simple global read/writes before/after this PR?

I have a hunch that programs before this PR are slightly faster, but only because I implemented globals without considering _ENV. But yes, I can try to create a few programs, and benchmark the interpreter on them!

[rbartlensky]

I would like to merge this PR before writing any benchmarks. I am going to implement functions and loops first, and then come back to optimizing the _ENV lookups.

[rbartlensky]

As an extra example, _ENV is handled in the following way:

Example program:

x = 2 -- equivalent to _ENV["x"] = 2

The compiler generates the following bytecode:

LDI     1 0   # load into register 1 integer 0
LDS     2 0   # load into register 2 string 0
SetAttr 0 2 1 # _ENV is always in register 0, _ENV["x"] = 2

and the tables:

Strings: { 0 -> "x" }
Integers: { 0 -> 2 }
Floats: {}

Every LuaString has an extra field which is the index of the string in the constant table. This field will be None if the user generates strings through concatenation.

The VM only executes set/get_attr if the loaded LuaString doesn't have an index. If it does, then the attribute is looked up via the env_attrs vector.

[ltratt]

I guess I was expecting something more like "GLOBAL_ASSIGN 0 2" where "0" == "print" and "1" is register 1 (which, presumably, holds the value 1). However, this is something we can optimise / change later.

[rbartlensky]

I guess I was expecting something more like "GLOBAL_ASSIGN 0 2" where "0" == "print" and "1" is register 1 (which, presumably, holds the value 1). However, this is something we can optimise / change later.

I see what you mean. That might speed things up, and the changes should be easy to make even later on.

[ltratt]

Is it worth using up a register for ENV? We could stick it in a special variable inside the VM?

[rbartlensky]

I would say yes, because most of the time you use functions from the standard library. I could introduce new instructions such as GET/SET_ENV, or have a special instruction which loads the _ENV into a variable, and then use GET/SET_ATTR on the register which has _ENV.

[ltratt]

Yes to which question?

[rbartlensky]

Oh sorry, yes to the first question. I think I am starting to see your point. But if I special case _ENV, that means that I will need special instructions as well, which will have _ENV as an implicit argument.

[ltratt]

Agreed. Let's leave that to a later PR.

[ltratt]

It feels a bit weird that nothing in the if arm references the left hand-side. The logic feels sort of inside-out to me.

[rbartlensky]

I don't see a reason why it should. The left hand-side is the register in which the value is copied. I am only interested to see if the value comes from _ENV or not. Either way, the value is copied here in the left hand-side. I hope I am not misunderstanding your comment.

[ltratt]

What I mean is that the logic here feels like it should be something like:

if arg2 == ENV_REG {
    if let Some(i) = attr.get_constant_index() {
        ...
    }
}

[rbartlensky]

Oh I see, but then I would only be able to write this:

        if arg2 == ENV_REG {
            if let Some(i) = attr.get_constant_index() {
                vm.env_attrs[i].clone()
            } else {
                from.get_attr(attr)?
            }
        } else {
            from.get_attr(attr)?
        }

I prefer my initial solution, but this might be slightly more efficient and more readable?

[rbartlensky]

I saw this merged, but I don't think it is part of stable. At least it doesn't work on rust playground (I made sure to check rust 2018).

[ltratt]

Maybe:

match attr.get_constant_index() {
   Some(i) => if arg2 == ENV_REG { vm.env_attrs[i].clone() } else { from.get_attr(attr)? }
   None => ...
}

[rbartlensky]

I will duplicate from.get_attr(attr)? this way. Is that fine?

[ltratt]

Your call. It feels clearer to me, but I don't feel strongly about it.

[ltratt]

Same comment as above.

[rbartlensky]

Ready for another review!

[ltratt]

Please squash.

[rbartlensky]

Squashed!