Pattern internals - AST version

[pderouen]

Again, this is a very early PR to show the direction I'm headed and give a chance for early feedback. When I got to the point of creating a node that could have an arbitrary number of children things got tricky. So, there's a collector module to implement a stack. My thought is that when a register or driver action begins a new collection Vec is pushed onto the stack to collect children (pin actions, etc.) as they occur. Then, when the driver or register action is done, the child collection is popped off the stack and added to the register action node. It would be nice to add a wrapper to automatically implement this. This setup should support arbitrarily deep nested nodes.

More thought is needed for the action structs. For example, pin and register names are strings. A numeric key would use less memory. Pin data will likely be stored using the .to_str() method that @coreyeng created. That gives a unique character representing what function is being done. The same could be done for registers.

Have a look. This is related to #18 and #19

[pderouen]

The test in collector.rs should give an idea of how to create and interact with nodes

[ginty]

Hi @pderouen, nice work!

Looks good to me so far and the collector system for adding new nodes within the currently 'open' node seems nice.
Re. the comment about automatically closing the currently open collection, you may want to google Rust callbacks.
I haven't used them myself yet, but the impression I got is that we can use them like Ruby blocks and wrap some sequence of operations with open and teardown logic which is kinda what you want here.

As you allude to, we will want to eventually converge on storing ids for pins, regs and other primary objects per #40, which will be more efficient for object retrieval as well as AST storage.

From your test cases it looks like you have already proven that this system will work to create an AST with the necessary linkages between nodes of different types, which is awesome.

I think the two remaining proof of concepts you should focus on before going too much further with more detailed implementation are:

• Does AST walking/interpretation work? i.e. you can now create a toy AST, can we create a toy interpreter to do something with it?
• How is the AST storage going to work when the AST is created from python API calls rather than from within Rust per your test case?

For the latter, the only way I have found it can really be done is via a global variable/static. In #40 I started using this id arena crate too, but I ended up shying away from it as I got worried about creating memory leaks.
My concern was that if you assigned this global to an arena instance (the current pattern in your case), there was no way to clear it when starting a new pattern.
Therefore you would have to instantiate a new arena and assign it to the global var, however what happens to all the memory used by the old arena in that case, does it get freed or not?

Maybe it does, but my rust-fu wasn't up to knowing for sure so I opted for a simpler system whereby a single vector instance is used for storage and I could clear that at will (which doesn't actually free the memory, but it ensures that for the next run I will be re-using the same memory and therefore not leaking).

I think you could take the same approach here if required, but I'm definitely not mandating that and no issue from me if you can resolve a way to make it work with the current id arena-based system.

Again, great progress so far!

[pderouen]

I was hoping to be able to use drop (https://stackoverflow.com/questions/42910662/is-it-possible-in-rust-to-delete-an-object-before-the-end-of-scope) to clear out the storage structure between patterns. Otherwise, there's another crate that the id-arena page recommends if you need deletion (I assume that was to delete individual nodes, which I don't think we need).

[ginty]

@pderouen, sounds good

[ginty]

And yeah, we shouldn't need to delete AST nodes, an AST should be considered immutable and to transform it in some way you create a new one.

[pderouen]

My concern was that if you assigned this global to an arena instance (the current pattern in your case), there was no way to clear it when starting a new pattern.
Therefore you would have to instantiate a new arena and assign it to the global var, however what happens to all the memory used by the old arena in that case, does it get freed or not?
Maybe it does, but my rust-fu wasn't up to knowing for sure so I opted for a simpler system whereby a single vector instance is used for storage and I could clear that at will (which doesn't actually free the memory, but it ensures that for the next run I will be re-using the same memory and therefore not leaking).

Clearing an Arena is much harder than I thought. I hacked a wrapper to do it. Can you point me to your solution? Sounds like something similar.

[ginty]

Hi @pderouen,

The arena basically boils down to giving you this:

id = my_arena.store(my_obj)
my_arena[id]   # => my_obj

i.e. a key/value store and it takes care of generating a unique ID for you.

I changed to using a std Vector instead, where I just pushed items on and used their index as their unique ID:

id = my_vector.len()
my_vector.push(my_obj)
my_vector[id]   # => my_obj

So pretty much the same API, but now you are dealing with a std vector as the store instead of the obfuscated arena store. Performance for fetch will be at least equivalent and likely a bit better.

Now when it comes to reloading a target it was easy for me now to just do my_vector.clear() and re-use it, rather than having to swap it for a new instance and worry about whether the memory associated with it was being freed or not.

This is all contained in https://github.com/Origen-SDK/o2/blob/master/rust/origen/src/core/dut.rs

You will see that the Dut struct contains the storage vectors, then there is a method that clears them when the DUT is changed (target reloaded).

The rest of that file contains a user API to add items to the collections and to get mutable and immutable references to the contained objects.
An API was used to give the flexibility to change the underlying storage in the future if we want to and to prevent exposing the ability to delete an item from the collection which would break the database by changing all the indexes.

So far, that approach seems to be working well.

[pderouen]

Well, I'm not sure what value the id-arena is adding when compared to the approach you mention (which is almost the same as the first implementation I did). I may change it again. 3rd time's a charm?

...get's easier every time.

[pderouen]

I think the two remaining proof of concepts you should focus on before going too much further with more detailed implementation are:

Does AST walking/interpretation work? i.e. you can now create a toy AST, can we create a toy interpreter to do something with it?

Is this what you wanted to see?

o2/rust/origen/src/core/pattern/collector.rs

Lines 148 to 167 in db3d9a7

	// iterate through the nodes to generate the final pattern output
	process_all_nodes(&pattern_nodes.main_collection(), &pattern_nodes);
	}
	// simple processor example - this will be deleted in the future
	fn process_all_nodes(nodes: &Collector, pattern: &NodeCollection) {
	for node_id in nodes.collection.iter(){
	if let Some(node) = pattern.nodes.get(*node_id) {
	match node {
	AstNode::Pin(pa) => println!("Call the method that updates the output pattern vector string, sending pa(PinAction struct)"),
	AstNode::Register(ra) => {
	println!("for an ascii pattern, print a comment with the register info, then process all children");
	process_all_nodes(&ra.children, pattern);
	},
	AstNode::Comment(comment) => println!("print the comment to the output"),
	_ => println!("other stuff"),
	}
	}
	}
	}

How is the AST storage going to work when the AST is created from python API calls rather than from within Rust per your test case?

I'll work on this next.

[ginty]

Hi @pderouen, yep this is looking really good!

[pderouen]

@ginty, my plan for closing out this PR (initial internal pattern storage structures) is to add in the Python interface (and some tests), clean up, and document. Sound good? Are you or anybody else waiting for this to move forward? ...It's coming along ever so slowly, learning as I go.

[ginty]

Sounds good.

I'm not waiting for it, but I'm feeling similarly that I'm blocking progress on registers, but this stuff is hard and its worth taking whatever time is needed to get these foundations right.

[pderouen]

I think I'll actually have time to tie a bow around this PR this week or next. Instead of making a temporary debug dump of the pattern AST to the console, I thought I might export it to a text file. Do we have a favorite format for the content in the file?

[ginty]

Cool.
I think S-expressions are a common and effective way to visualize an AST, see the examples in this README - https://github.com/Origen-SDK/atp

[pderouen]

closing this PR. A complete re-write is needed.