Do not generate unused code for BinAST parser.

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks 1 bug)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

8 months ago
in BinSource-auto.{cpp,h}, we're generating method for all interfaces (if I understand correctly),
but some methods are not used because we replace the code for field in yaml,
leaving such unused method in the code is troublesome for coverage test.

we could tweak the code generator to calculate the set of used methods,
and not generate unused code.

maybe we need to also tweak the yaml file to declare the set of manually called methods.
(to create the correct graph)
Assignee

Comment 1

8 months ago
the large part of them should be "parseX vs parseInterfaceX" interacts badly with sum interface.
we shouldn't generate parseX if X is used only from sum interface.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee

Comment 2

8 months ago
added reference graph calculation and tracing in code generator.

currently the reference graph doesn't check `replace` field,
because the set of used methods doesn't increase by supporting it,
which means we can leave it to other bug.

-3k lines in auto-generated code :)
Attachment #9013931 - Flags: review?(dteller)
Comment on attachment 9013931 [details] [diff] [review]
Do not generate unused code for BinAST parser.

Review of attachment 9013931 [details] [diff] [review]:
-----------------------------------------------------------------

Nice cleanup in the generated code :)

::: js/src/frontend/binsource/src/main.rs
@@ +490,5 @@
>      /// Always use MOZ_TRY_VAR regardless of the result type.
>      AlwaysVar,
>  }
>  
> +/// Fixed parameter of interface method.

This file starts to look biiig.

Any chance you could move the `ReferenceGraph` definition and implementation to a new module?

@@ +509,5 @@
> +}
> +
> +/// A reference graph of the method call.
> +///
> +/// All auto-generated methods has corresponding node in this reference graph,

Nit: "All" => "Each"

@@ +518,5 @@
> +///   * we generate parseX and parseInterfaceX for `interface X`
> +///   * parseX is not used if `interface X` appears only in sum interface
> +struct ReferenceGraph {
> +    /// The map from the node name to node struct.
> +    refnodes: HashMap<String, Rc<ReferenceGraphNode>>,

Rc<String>?

@@ +528,5 @@
> +        }
> +    }
> +
> +    /// Mark all nodes from node with `name` as used.
> +    fn trace(&mut self, name: &String) {

Here, too, Rc<String>?

@@ +532,5 @@
> +    fn trace(&mut self, name: &String) {
> +        let mut edges: HashSet<String> = HashSet::new();
> +        edges.insert(name.clone());
> +
> +        loop {

Nit: Bonus points if you give this loop a name :)

@@ +533,5 @@
> +        let mut edges: HashSet<String> = HashSet::new();
> +        edges.insert(name.clone());
> +
> +        loop {
> +            let mut next_edges: HashSet<String> = HashSet::new();

Rc<String> :)

@@ +537,5 @@
> +            let mut next_edges: HashSet<String> = HashSet::new();
> +
> +            for edge in edges {
> +                let refnode = self.refnodes.get_mut(&edge)
> +                    .expect("Referred node should exist");

What does it mean if it doesn't exist? Would it be useful to display the node name in case of problem?

let refnode = ...
   .unwrap_or_else(|| panic!("While computing dependencies, node {} doesn't exist", edge));

@@ +538,5 @@
> +
> +            for edge in edges {
> +                let refnode = self.refnodes.get_mut(&edge)
> +                    .expect("Referred node should exist");
> +                if !refnode.used {

Nit: The usual Rust guidelines match our C++ guidelines, so something along the lines of:

if refnode.used {
  continue;
}

?

@@ +541,5 @@
> +                    .expect("Referred node should exist");
> +                if !refnode.used {
> +                    *refnode = Rc::new(ReferenceGraphNode {
> +                        used: true,
> +                        edges: refnode.edges.clone()

Looks like you should rather make `used` a `RefCell<bool>`. This would let you modify in place, instead of cloning.

@@ +544,5 @@
> +                        used: true,
> +                        edges: refnode.edges.clone()
> +                    });
> +
> +                    for next_edge in refnode.edges.clone() {

No need to clone here, either :)

for next_edge in refnode.edges.iter() {
 // ...
}

@@ +559,5 @@
> +        }
> +    }
> +
> +    /// Returns true if the node with `name` is used.
> +    fn is_used(&self, name: &NodeName) -> bool {

NodeName contains a Rc<String> \o/

@@ +565,5 @@
> +            Some(refnode) => refnode.used,
> +            None => false
> +        }
> +    }
> +    /// Returns true if the node with `prefix+name` is used.

Could you give an example?

@@ +574,5 @@
> +        }
> +    }
> +
> +    /// Insert a node with `name` to the graph.
> +    fn insert(&mut self, name: &String, refnode: Rc<ReferenceGraphNode>) {

Are there any cases in which the node needs to be created by the caller? If not, `insert` could create the `ReferenceGraphNode` itself.

@@ +578,5 @@
> +    fn insert(&mut self, name: &String, refnode: Rc<ReferenceGraphNode>) {
> +        self.refnodes.insert(name.clone(), refnode);
> +    }
> +
> +    /// Insert a node with `prefix+name` to the graph.

We should probably leave contatenations outside and just take a Rc<String>.

@@ +691,5 @@
>  
> +    fn generate_reference_graph(&mut self) {
> +        let mut refgraph = ReferenceGraph::new();
> +
> +        // FIXME: Check replace rule and modify the reference (bug NNNNNN).

What does this mean?

@@ +696,5 @@
> +
> +        // 1. Typesums
> +        let sums_of_interfaces = self.syntax.resolved_sums_of_interfaces_by_name();
> +        for (name, nodes) in sums_of_interfaces.clone() {
> +            let mut edges: HashSet<String> = HashSet::new();

Rc<String> (I'll stop mentioning it now)

@@ +705,5 @@
> +            }));
> +
> +            let mut sum_edges: HashSet<String> = HashSet::new();
> +            for node in nodes {
> +                sum_edges.insert(format!("Interface{}", node.to_string().clone()));

You don't need to clone it here.

@@ +731,5 @@
> +                    | None => {
> +                        let typename = TypeName::type_(field.type_());
> +                        interface_edges.insert(typename.to_string());
> +                    },
> +                    _ => {}

Could you document this?

@@ +799,5 @@
> +
> +        self.refgraph = refgraph;
> +    }
> +
> +    fn trace(&mut self, name: &String) {

The role of that string is unclear.

@@ +1097,4 @@
>                                                    &extra_params);
> +
> +            if self.refgraph.is_used(name) {
> +                outer_parsers.push(outer.reindent(""));

We could simply avoid generating `outer` if that's not the case, right?

@@ +1100,5 @@
> +                outer_parsers.push(outer.reindent(""));
> +            }
> +
> +            if self.refgraph.is_used_prefix(inner_prefix, name) {
> +                inner_parsers.push(inner.reindent(""));

Same here.

@@ +2005,5 @@
>      let global_rules = GlobalRules::new(&new_syntax, &yaml[0]);
> +    let mut exporter = CPPExporter::new(new_syntax, global_rules);
> +
> +    exporter.generate_reference_graph();
> +    exporter.trace(&"Program".to_string());

Could you make this a named constant?
Attachment #9013931 - Flags: review?(dteller) → feedback+
Assignee

Comment 4

8 months ago
Moved the ReferenceGraph* to refgraph.rs.

Mostly replaced String/NodeName with Rc<String>.
then, I still need to clone String when creating Rc<String> from NodeName, because NodeName doesn't have method that returns Rc<String>.
I'll fix it in binjs-ref later, and fix the consumer in m-c after that
(I want to land this to improve the code coverage accuracy)
Attachment #9013931 - Attachment is obsolete: true
Attachment #9014245 - Flags: review?(dteller)
Comment on attachment 9014245 [details] [diff] [review]
Do not generate unused code for BinAST parser.

Review of attachment 9014245 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/binsource/src/main.rs
@@ +510,5 @@
> +/// Get Rc<String> from NodeName.
> +///
> +/// FIXME: Do not clone the String itself, but just clone the Rc<String> inside
> +///        NodeName (Bug NNNNNN).
> +fn cell_from_nodename(name: &NodeName) -> Rc<String> {

Nit: That's not a cell :)

@@ +657,5 @@
> +                        let typename = TypeName::type_(field.type_());
> +                        interface_edges.insert(Rc::new(typename.to_string()));
> +                    },
> +
> +                    // Don't have to handle other cases (string, number, etc).

Nit: Could you just add the word "field" somewhere in that comment? :)

::: js/src/frontend/binsource/src/refgraph.rs
@@ +44,5 @@
> +    /// as used. `name` is the name of the method, without leading "parse".
> +    pub fn trace(&mut self, name: Rc<String>) {
> +        // The set of edges to trace in the current iteration.
> +        let mut edges: HashSet<Rc<String>> = HashSet::new();
> +        edges.insert(name.clone());

Nit: I think that you actually don't need to clone here.

@@ +84,5 @@
> +    }
> +
> +    /// Insert a node with `name` to the graph.
> +    pub fn insert(&mut self, name: Rc<String>, edges: HashSet<Rc<String>>) {
> +        self.refnodes.insert(name.clone(),

Nit: Here, too, no need to clone.
Attachment #9014245 - Flags: review?(dteller) → review+

Comment 7

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee9161a4b37a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.