myst-lang / myst

A structured, dynamic, general-purpose language.
http://myst-lang.org
MIT License
119 stars 17 forks source link

Properly encapsulate types within the Interpreter #18

Closed faultyserver closed 7 years ago

faultyserver commented 7 years ago

As noted in the description of 5628e61, all of the Types for the value classes and native libraries are currently implemented as constants.

For most applications this is okay, as the Interpreter will only be instantiated once. However, this causes issues for the tests and hinders the ability to embed the interpreter in other applications. Additionally, it's simply a bad practice, breaking the encapsulation of the Interpreter class.

I really don't know how to go about fixing this. I've spent 3 days thinking about how to move things around to allow the types to be dynamically allocated, but I haven't found a way to make those dynamically-created types work with the .type that each Value type has. I think the solution will involve removing .type and doing type resolution somewhere else.

As an example of the problem, take a look at these two tests:

it_raises %q(
  deftype Map
    def to_s
      "a map"
    end
  end

  raise {}
),                          "a map"

it_raises %q(
  deftype Map
    def to_s
      "different text"
    end
  end

  raise {}
),                          "different text"

(it_raises is just a macro that runs the code and expects the second argument in the error output after interpreting).

The second test here will fail, because the previous definition of Map#to_s will still exist when the second test starts, and because clauses are appended to functors rather than prepended, that first definition will be matched when raise calls to_s, leading to incorrect output.

FWIW, this only affects the native types and modules, as they are the only types/modules that are maintained as constants rather than defined dynamically.

faultyserver commented 7 years ago

Potentially a "bigger picture" view of this is that the Value types are doing to much. Rather than just being data storage, they have behavior tied to them, including scopes, ancestries, included modules, etc.

This issue on charly-lang makes a good point that performance can be improved by not wrapping the native types and instead relying directly on Crystal's Int64, Float64, etc (note that Symbols would still need a wrapped type to support dynamic creation and referencing).

This is part of what I was thinking with the "removing .type and doing type resolution somewhere else". If Value was just an alias for all of the different types (natives, lists, maps, modules, types, instances, functors, callables), then there could be a method in the interpreter that determines .type based on the actual type of the Value, rather than the other way around. This seems like a more appropriate order of responsibility.

I'm concerned with how much rewriting this will take, but it seems necessary to resolve this issue.

faultyserver commented 7 years ago

I'm a little surprised by the performance impact this last commit had on the interpreter (30% slow down). I don't know if it's because of the dynamically-generated kernel, or the dynamic type resolution. I'm going to investigate some more to see what can be improved.

faultyserver commented 7 years ago

After further investigation (despite the claims of the above commit), the majority of the added time is spent instantiating an Interpreter for every test that creates one. The added overhead comes from instantiating the kernel property for each one, where all of the native methods are re-created. This is both a waste of memory and time, but accounts for all of the slow down that I've seen in the tests.

In practice, this is entirely negligible. The test suite creates more than 1000 Interpreters, the execution of which takes <50ms. That means that the average Interpreter instantiation only takes ~50µs, which is perfectly fine.

I might try to refactor the methods to be constant references instead, but I don't think that's necessary to close this issue.

Finally, the added point of not wrapping Crystal's types ended up causing more issues than it resolved, so I'll be leaving it out for now. It will likely come back in the future.