Reduce redundant closurizing when concatenating Arrays. #717
Conversation
This makes it so array concatenation doesn't need to call closurize, by introducing a flag in ArrayAttrs. We still assume in most of the code that Arrays (especially thoses generated by the interpreter) are not closurized, though there may be room for improvement there. In order to hide the added ArrayAttrs during serialization (and set it to a sane default in deserialization) we need a custom implementation like the ones for Num and Record.
This is mostly done for instances where we create Array Str; and for when the interpreter inserts an already closurized Array.
| /// Serialize for an Array. Required to hide the internal attributes. | ||
| pub fn serialize_array<S>( | ||
| terms: &Vec<RichTerm>, | ||
| _attrs: &ArrayAttrs, | ||
| serializer: S, | ||
| ) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: Serializer, | ||
| { | ||
| let mut seq = serializer.serialize_seq(Some(terms.len()))?; | ||
| for term in terms.iter() { | ||
| seq.serialize_element(term)?; | ||
| } | ||
|
|
||
| seq.end() | ||
| } | ||
|
|
||
| /// Deserialize for an Array. Required to set the default attributes. | ||
| pub fn deserialize_array<'de, D>(deserializer: D) -> Result<(Vec<RichTerm>, ArrayAttrs), D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let terms: Vec<RichTerm> = Vec::deserialize(deserializer)?; | ||
| Ok((terms, Default::default())) | ||
| } | ||
|
|
There was a problem hiding this comment.
You may actually not need this code, because #[serde(skip)] seems to work for variant fields of tuple as well, so you should be able to do instead:
RichTerm {
...
Array(Vec<RichTerm>, #[serde(skip)] ArrayAttrs)
...
}There was a problem hiding this comment.
Hm. It seems array serialization tests are failing, because of an extra [] added by serde.
So instead of { "bar": ["str", true] } w get { "bar": [ ["str", true] ] }. Maybe this happened because we now have a two-element tuple instead of one vector. I'll look into serde attributes.
This will be useful once Records are closurized in the eval loop.
|
I think the benchmark runner still fails with the classic |
e04f142
to
6fa167a
Compare
The important bit is the match arm in
eval::eval_closurewhich uses a new attribute set inArrayterms to remember when an array was closurized. This is then exploited in theArrayConcatoperation to avoid needlessly callingclosurizeon expressions of the forma1 @ a2 @ ... @ an.In order to preserve (de)serialization of Arrays we need custom implementations like those of
NumandRecord; this is becauseserdewill simply insert the extra attributes in the output and expect them in the input even though, in this case, they are just implementation details.And because of this, testing is a bit more involved, for example the
evaluation_fulltest initially failed for me because it compared equality of a parsing result (not closurized) to full evaluation result (closurized). If we write more of these kinds of tests, we may need some helper functions for this.