search

facebook / starlark-rust

A Rust implementation of the Starlark language
Apache License 2.0
707 stars 57 forks source link

Multiple evaluations with same AST #124

Closed monoid closed 1 month ago

monoid commented 3 months ago

Currently eval_module consumes AstModule instance, so one has to parse the script for each invocation, making it impossible to cache the AST. You cannot even clone an AstModule instance.

ndmitchell commented 3 months ago

The original reason was that the AST contains a number of fields such as Name(String), and by consuming the AST we can evaluate without reallocating those names. Now we have moved on to a bytecode compiler, I'm less convinced by that reasoning. In the meantime, supporting Clone doesn't seem unreasonable either. CC @stepancheg for thoughts.

ndmitchell commented 2 months ago

I took a look into this. The evaluator takes the AST by value, since the first thing it does is annotate a lot of the nodes in the AST. It does that by consuming the input AST and producing a new AST tagged with the right data. That means that while the evaluation doesn't really consume the AST, it would be a lot of effort to change it to not consume it.

Fortunately adding a Clone to AstModule isn't hard. I've put up a diff internally for that.

ndmitchell commented 2 months ago

9b6115728cf79bee208c167aef093456f1677714 now adds Clone, so if you want to do multiple evaluations, just clone it.

monoid commented 2 months ago

@ndmitchell thanks! I will try to benchmark it...

monoid commented 2 months ago

Well, the profit on the ./benchmark/benchmark.py is clear:

parsing AST             time:   [53.628 µs 53.644 µs 53.660 µs]
Found 7 outliers among 100 measurements (7.00%)
  2 (2.00%) high mild
  5 (5.00%) high severe

cloning AST             time:   [4.8612 µs 4.8755 µs 4.8877 µs]

The ./starlark/hello_world.star demonstrates similar improvement.

ndmitchell commented 1 month ago

Fantastic - can we close this issue then?

monoid commented 1 month ago

Yes, thank you!