Improve logic around data directive references #261

wfchandler · 2018-02-17T05:00:19Z

The present assembly filter is a bit overeager with label references in data directives. This results in labels being included in filtered results that are not referenced by an opcode, directly or indirectly.

This change adds a check to ensure that label references from data directive are only treated as used if the label the data directive is part of is used by an opcode, or referenced by a data directive that is itself used by an opcode.

This should also be the final polish for the current filtering logic, at least until I notice another bug 😄

shepmaster · 2018-02-17T15:20:40Z

ui/src/asm_cleanup.rs

            for label_ref_cap in LABEL_REF_REGEX.captures_iter(line).skip(1).filter_map(|cap| cap.get(1)) {
-                opcode_operands.insert(label_ref_cap.as_str());
+                // Capture unique labels referenced by data dirs in the current label
+                let label_set = data_dir_label_refs.entry(current_label).or_insert(HashSet::new());


.or_insert_with(HashSet::new)

What's the advantage of HashSet::new over HashSet::new() here?

It avoids creating the HashSet when the value already exists.

In this case, it's not a big benefit as an empty HashSet doesn't allocate memory and is just a few pointers-worth of stack data. In other cases, it can have strong performance implications I (and clippy!) prefer it this way to be clear that the code only executes when needed.

I think I understand, the function in or_insert is eagerly evaluated, but or_insert_with is lazy, right?

If that's correct, I might put a PR in for the entry API docs to clarify this; I had to look at the "or" method under Result for the implications to be spelled out.

the function in or_insert is eagerly evaluated, but or_insert_with is lazy, right?

That's right, except that or_insert doesn't take a function/closure, but only a value, so it must be evaluated.

shepmaster · 2018-02-17T15:20:56Z

ui/src/asm_cleanup.rs

-                opcode_operands.insert(label_ref_cap.as_str());
+                // Capture unique labels referenced by data dirs in the current label
+                let label_set = data_dir_label_refs.entry(current_label).or_insert(HashSet::new());
+                &label_set.insert(label_ref_cap.as_str());


Why is a reference being taken here?

Mistake, I misread the docs for or_insert.

shepmaster · 2018-02-17T15:25:18Z

ui/src/asm_cleanup.rs

+    let mut iteration_count = 0; 
+    const MAX_ITERATIONS: u32 = 100; // Safeguard against hangs, should not be an issue but better to be safe
+
+    while new_used_label_found && iteration_count < MAX_ITERATIONS {


Since the iteration limit is a safety concern, let's promote it to the primary loop condition and make it less prone to failure. Untested changes:

let mut checked_labels = HashSet::new(); const MAX_ITERATIONS: u32 = 100; // Safeguard against hangs, should not be an issue but better to be safe for _ in 0..MAX_ITERATIONS { let mut new_used_label_found = false; for (label, references) in &data_dir_label_refs { if used_labels.contains(label) && checked_labels.insert(label) { new_used_label_found = true; for referenced_label in references.iter() { used_labels.insert(referenced_label); } } } if !new_used_label_found { break; } }

That's a big improvement, thanks!

To be clear, I have not experienced any hangs in my testing and in practice this whole check only adds a 5% performance penalty, but I worry about someone compiling a truly massive file that interacts with this loop in ways I did not anticipate.

shepmaster · 2018-02-17T15:28:57Z

ui/src/asm_cleanup.rs

@@ -73,7 +74,8 @@ pub fn filter_asm(block: &str) -> String {
    let mut current_label: &str = "";
    let mut line_info: Vec<LineType> = Vec::new();
    let mut labels: HashSet<&str> = HashSet::new();
-    let mut opcode_operands: HashSet<&str> = HashSet::new();
+    let mut opcode_operands: HashSet<&str> = HashSet::new(); 
+    let mut data_dir_label_refs: HashMap<&str, HashSet<&str>> = HashMap::new(); 


FWIW, you don't have to change this in this PR, but I'll probably go though here with Clippy and remove a bunch of the explicit types on variable declarations — they aren't very idiomatic.

shepmaster · 2018-02-17T15:29:14Z

ui/src/asm_cleanup.rs

@@ -73,7 +74,8 @@ pub fn filter_asm(block: &str) -> String {
    let mut current_label: &str = "";
    let mut line_info: Vec<LineType> = Vec::new();
    let mut labels: HashSet<&str> = HashSet::new();
-    let mut opcode_operands: HashSet<&str> = HashSet::new();


Why is this line changing? Is there extra whitespace?

Yup, must have added an extra space. Removing.

shepmaster · 2018-02-17T15:29:58Z

ui/src/asm_cleanup.rs

@@ -4,6 +4,7 @@
 use regex::Regex;
 use regex::Captures;
 use rustc_demangle::demangle;
+use std::collections::HashMap;


FWIW, you don't have to change this in this PR, but I'll probably rewrite these as std::collections::{HashMap, HashSet} at a later point;

I'll condense the collections and regex imports.

shepmaster · 2018-02-17T19:57:55Z

ui/src/asm_cleanup.rs

+            if used_labels.contains(label) && checked_labels.insert(label) {
+                new_used_label_found = true;
+
+                for referenced_label in references.iter() {


Maybe just used_labels.extend(&references)?

Yes, that's much better. I was bothered by the nested for loops, but wasn't coming up with anything more expressive.

I decided to just move references into used_labels instead of passing a reference, as it's not used again in the HashMap.

shepmaster · 2018-02-17T23:27:11Z

ui/src/asm_cleanup.rs

+                break;
+            }
+        }
+    }


I'm afraid I've busted your logic here... or at least I don't understand it anymore.

On the first iteration of the for loop, new_used_label_found is false. So long as we find the current label, the loop will continue, but as soon as we hit the first one, the loop stops. I think it's equivalent to this:

for _ in 0..MAX_ITERATIONS { for (label, references) in &data_dir_label_refs { if used_labels.contains(label) && checked_labels.insert(label) { new_used_label_found = true; used_labels.extend(references); } else { break; } } }

This seems a little strange that we'd exit the inner loop like that; maybe the if !new_used_label_found should be outside the for?

Is this algorithm essentially trying to find the reachable nodes of a graph starting from a set of nodes? If so, you might want to look at petgraph::GraphMap instead of rebuilding a graph by hand. It may even have an algorithm for finding the reachable nodes...

Sloppy mistake, my apologies.

Yes, your summary is correct. I'll look into petgraph, it should be an interesting to try something new.

That was much less painful than I was expecting, thanks for the suggestion!

shepmaster · 2018-02-18T15:22:47Z

ui/src/asm_cleanup.rs

+    used_labels.iter()
+        .filter(|label| label_graph.contains_node(label))
+        .flat_map(|n| label_graph.neighbors_directed(n, Outgoing))
+        .for_each(|node| data_labels.push(node));


Very succinct! Could this now be

let data_labels: Vec<_> = used_labels.iter() .filter(|label| label_graph.contains_node(label)) .flat_map(|n| label_graph.neighbors_directed(n, Outgoing)) .collect();

I noticed the loop is gone; do we no longer need to deeply trace the graph?

Gah, you're right! I apologize, let me take another pass at this.

Thanks for putting up with all my pedantic quibbles ;-)

I'm guessing the loop can be broken as:

if data_labels.is_empty() { break; }

Actually, it's probably if used_labels doesn't change size when the data_labels are added...

I don't consider "not working correctly" to be a pedantic quibble 😄

It should be checking the full graph now, and I added a test, which I should have done earlier.

shepmaster

That's some nice crate usage!

wfchandler · 2018-02-19T19:44:24Z

Thanks! As always, thank you for all your help and patience.

shepmaster reviewed Feb 17, 2018

View reviewed changes

wfchandler force-pushed the fix_data_directives branch from 2947900 to 7ae82da Compare February 17, 2018 19:07

shepmaster reviewed Feb 17, 2018

View reviewed changes

wfchandler force-pushed the fix_data_directives branch from 7ae82da to 2e8afca Compare February 17, 2018 21:20

shepmaster reviewed Feb 17, 2018

View reviewed changes

wfchandler force-pushed the fix_data_directives branch 2 times, most recently from 5c2c297 to 1cbd19d Compare February 18, 2018 14:43

shepmaster reviewed Feb 18, 2018

View reviewed changes

Improved logic around data directive references

0d75432

wfchandler force-pushed the fix_data_directives branch from 1cbd19d to 0d75432 Compare February 19, 2018 01:10

shepmaster approved these changes Feb 19, 2018

View reviewed changes

shepmaster merged commit dcbce54 into rust-lang:master Feb 19, 2018

wfchandler deleted the fix_data_directives branch March 7, 2018 04:29

Improve logic around data directive references #261

Improve logic around data directive references #261

Uh oh!

Conversation

wfchandler commented Feb 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shepmaster Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shepmaster left a comment

Choose a reason for hiding this comment

Uh oh!

wfchandler commented Feb 19, 2018

Uh oh!

Uh oh!

shepmaster Feb 17, 2018 •

edited

Loading