Skip to content

Too much boxing since 472aad50 (#361) #362

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

Closed
Ten0 opened this issue Apr 13, 2021 · 5 comments · Fixed by #363
Closed

Too much boxing since 472aad50 (#361) #362

Ten0 opened this issue Apr 13, 2021 · 5 comments · Fixed by #363

Comments

@Ten0
Copy link
Contributor

Ten0 commented Apr 13, 2021

It looks like #361 introduced a bug where it incorrectly considers that types are recursive even though they aren't, if one type appears twice in the tree but it's still a tree. This generates unnecessary Boxes.

@Ten0
Copy link
Contributor Author

Ten0 commented Apr 13, 2021

@noverby

@Ten0
Copy link
Contributor Author

Ten0 commented Apr 13, 2021

Additionally it looks like the String alloc is not necessary: this compiles:

    fn contains_type_without_indirection<'a>(
        &'a self,
        input_id: InputId,
        schema: &'a Schema,
        visited_types: &mut HashSet<&'a str>,
    ) -> bool {
        visited_types.insert(&self.name);
        // The input type is recursive if any of its members contains it, without indirection
        self.fields.iter().any(|(_name, field_type)| {
            // the field is indirected, so no boxing is needed
            if field_type.is_indirected() {
                return false;
            }

            let field_input_id = field_type.id.as_input_id();

            if let Some(field_input_id) = field_input_id {
                if field_input_id == input_id {
                    return true;
                }

                let input = schema.get_input(field_input_id);

                // no need to visit type twice (prevents infinite recursion)
                if visited_types.contains(&input.name.as_str()) {
                    return true;
                }

                // we check if the other input contains this one (without indirection)
                input.contains_type_without_indirection(input_id, schema, visited_types)
            } else {
                // the field is not referring to an input type
                false
            }
        })
    }
}

pub(crate) fn input_is_recursive_without_indirection(input_id: InputId, schema: &Schema) -> bool {
    let input = schema.get_input(input_id);
    let mut visited_types = HashSet::<&str>::new();
    input.contains_type_without_indirection(input_id, schema, &mut visited_types)
}

@Ten0
Copy link
Contributor Author

Ten0 commented Apr 13, 2021

Looks like this should just be:

                // no need to visit type twice (prevents infinite recursion)
                if visited_types.contains(&input.name.as_str()) {
                    return false;
                }

as if we re-encounter the type we don't need to re-consider it as we have already done it before

@noverby
Copy link
Contributor

noverby commented Apr 13, 2021

@Ten0 ok will you create a PR or should I?

@Ten0
Copy link
Contributor Author

Ten0 commented Apr 13, 2021

I got the code patched already so I'll open it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants