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

IR - Test adding PrintItem::Items and PrintItems::RcItems #55

Closed
dsherret opened this issue Jan 21, 2020 · 2 comments
Closed

IR - Test adding PrintItem::Items and PrintItems::RcItems #55

dsherret opened this issue Jan 21, 2020 · 2 comments
Labels
performance wontdo This will not be worked on

Comments

@dsherret
Copy link
Member

dsherret commented Jan 21, 2020

PrintItems::RcItems

There are certain cases where cloning is necessary in the IR generation:

Condition::new("debuggingName", ConditionProperties {
    condition: Box::new(|context| true_or_false),
    true_path: Some(with_indent(items.clone())),
    false_path: Some(items),
});

It would be useful for performance reasons to instead write this as:

let items = Rc::new(items);
Condition::new("debuggingName", ConditionProperties {
    condition: Box::new(|context| true_or_false),
    true_path: Some(with_indent(items.into().clone())), // only an Rc clone, so should be fast
    false_path: Some(items.into()),
});

PrintItems::Items

Instead of writing this:

items.extend(parse_node(...));

It might possibly be faster to write this:

items.push(parse_node(...).into());
// short for
items.push(PrintItem::Items(parse_node(...)));

This may make the printer run slower, but maybe it might make the parser run faster? I'm not sure how vectors are in rust and how they perform, but this would be good to test out.

@dsherret
Copy link
Member Author

dsherret commented Jan 23, 2020

Not doing this. Instead I'm constructing a graph while parsing the nodes. Should be a lot faster (hopefully) and the parsing API will be better (going back to Signal::X too).

Looks like this:

let mut items = PrintItems::new();

items.push_info(start_info);
items.push_str("class");
items.extend(parse_node(....));
items.push_signal(Signal::NewLine);

items

...which internally builds up graph nodes and connects them together.

@dsherret dsherret added the wontdo This will not be worked on label Jan 27, 2020
@dsherret
Copy link
Member Author

The way better solution was implemented in #56.

Basically when parsing a directional graph is constructed. All paths in that graph are behind an Rc<T> so the paths can be shared. The printer then maintains a "next node stack" to figure out what the next node should be when reaching the end of a path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance wontdo This will not be worked on
Projects
None yet
Development

No branches or pull requests

1 participant