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

Speed up Comparison #497

Merged
merged 43 commits into from
Aug 21, 2024
Merged

Speed up Comparison #497

merged 43 commits into from
Aug 21, 2024

Conversation

yonnorc42
Copy link
Contributor

@yonnorc42 yonnorc42 commented Jul 26, 2024

I'm starting this pull request now but it's going to be a draft for a while until I've figured out exactly what changes we want to make and how to make them.

Current changes:

  • Matches as sets instead of lists

  • Passing in instance names instead of instances themselves, means unpacking the cache is a lot faster

  • Narrowing down list of possible matches through number of const nets (partially working, but not completely because of the issue with the const generator luts)

  • Checking pins based in order of if they're a const pin or not. The thought here is that a non const net connected to a pin has the ability to narrow the possibilities down a lot more than a const net does.

  • Breaks down some bigger functions to smaller functions

  • Flow argument logging_level which decides which severity of logging messages to print to the logs

  • named_netlist.instances_to_map holds tuples now, which contain the instance_name and the mapping function they should utilize. This cuts out later string comparisons.

  • Reorganization of netlist cleaner

@yonnorc42
Copy link
Contributor Author

yonnorc42 commented Jul 26, 2024

One of the changes I've made might dramatically slow down a run the first time, but should increase the speed in subsequent runs since the list of possible matches for each instance should hopefully be significantly shorter so comparison will be faster and the caching will be faster... as long as I get it working.

@yonnorc42
Copy link
Contributor Author

While using the cache, the runtime of jpegencode through comparison is about 16 minutes.

@reillymck
Copy link
Contributor

I was thinking about what you said in the meeting about the dictionary lookup times to for {instance_name: instance} dictionary. Since this datatype does not change, we should use a list. The potential matching sets should just have the list indices for the master instances list (instead of using the instance name keys).. Python list access should be O(1)
What do you think?

@yonnorc42
Copy link
Contributor Author

yonnorc42 commented Aug 5, 2024

Looking at the profile, I don't think it is actually taking as long as I thought. It was only a quarter of a second in the entire jpegencode run.
@reillymck
3244748 0.246 0.000 0.246 0.000 {method '__getitem__' of 'dict' objects}

Edit:
Actually this might be the line in the profile:

1500319645 103.563 0.000 103.563 0.000 {method 'get' of 'dict' objects}

I'm not using the get method, just the built in access method, so I'm not sure which one of these it actually is.

The only time get is used on dicts is a line which was already in the code previous to any of my changes.

@yonnorc42
Copy link
Contributor Author

Also, from what I'm seeing online, dict access is also about O(1)

@yonnorc42 yonnorc42 marked this pull request as ready for review August 14, 2024 03:21
Copy link
Contributor

@reillymck reillymck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is looking pretty good. Can you upload the latest profile results?

dictionary [] access should be get_item. I looked and I think the get() calls are spydrnet.
Dictionary access is O(1), since the time it takes to hash the key doesn't change based on the size of the datatype. But index access of course would be less instructions. However, I do not think we are impacted by this enough to care based on the get_item profile.

bfasst/utils/structural.py Outdated Show resolved Hide resolved
bfasst/utils/structural.py Outdated Show resolved Hide resolved
bfasst/utils/structural.py Outdated Show resolved Hide resolved
bfasst/utils/structural.py Outdated Show resolved Hide resolved
@yonnorc42 yonnorc42 marked this pull request as draft August 17, 2024 01:04
@yonnorc42
Copy link
Contributor Author

Going to push a few minor netlist_cleanup changes as well, same logic slightly different format

@yonnorc42
Copy link
Contributor Author

yonnorc42 commented Aug 19, 2024

Just added the flow argument logging_level to work with these tools: netlist_cleanup, error injector, phys netlist, and structural comparison.

Example flow run:

python scripts/run.py VivadoPhysNetlistCmp --flow_arguments "{'logging_level': 'ERROR'}"

Will only log messages with logging.error or above, so no logging.debug, logging.info, or logging.warning messages will get added to the log file. The options are DEBUG, INFO, WARNING, ERROR, CRITICAL

@yonnorc42 yonnorc42 marked this pull request as ready for review August 21, 2024 05:10
@yonnorc42
Copy link
Contributor Author

This should be good to merge @reillymck or we can talk about it during the meeting if there's anything else to be added

@reillymck reillymck merged commit 158a5d6 into main Aug 21, 2024
18 checks passed
@reillymck reillymck deleted the speed_up_cmp branch August 21, 2024 20:19
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 this pull request may close these issues.

2 participants