Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly encapsulate types within the Interpreter #18

Closed
faultyserver opened this issue Oct 23, 2017 · 3 comments
Closed

Properly encapsulate types within the Interpreter #18

faultyserver opened this issue Oct 23, 2017 · 3 comments
Labels
bug.interpreter A bug specifically relating to the interpreter source code bug Generic label for bugs. Every bug should have this tag in addition to the more specific bug tag rfc A request for comments from the community

Comments

@faultyserver
Copy link
Member

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 faultyserver added bug Generic label for bugs. Every bug should have this tag in addition to the more specific bug tag bug.interpreter A bug specifically relating to the interpreter source code rfc A request for comments from the community labels Oct 23, 2017
@faultyserver
Copy link
Member Author

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 added a commit that referenced this issue Oct 24, 2017
Instead of having each `Value` type define `.type`, the type is now determined via a call to `__typeof` (and the scope via `__scopeof`). This allows the interpreter to be fully encapsulated, meaning that programs can freely modify the Kernel types (e.g., define new methods on `List`) without affecting other instances of the Interpreter.
@faultyserver
Copy link
Member Author

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 added a commit that referenced this issue Oct 24, 2017
…solution.

Rather than having a cascade of tests to determine which type to look up, each type now has a name associated with it to use as the key for the type lookup.

For consistency, _every_ value type has a `type_name` method, but in practice, TType, TModule, and TInstance will not use it.
@faultyserver
Copy link
Member Author

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.

faultyserver added a commit that referenced this issue Oct 24, 2017
This was the original reason for #18. Take the following code:

```
defmodule Enumerable
end

deftype List
  include Enumerable
end
```

This used to fail because the `List` type did not have a parent scope, and so it could not find the `Enumerable` module to include. Now, with the dynamic types for values, the Kernel scope can be injected as the parent for each instance of the Interpreter. That allows the above code to properly resolve `Enumerable` and work as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug.interpreter A bug specifically relating to the interpreter source code bug Generic label for bugs. Every bug should have this tag in addition to the more specific bug tag rfc A request for comments from the community
Projects
None yet
Development

No branches or pull requests

1 participant