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

Separate physical and logical layer into different crates #81

Closed
aannleax opened this issue Jan 4, 2023 · 4 comments
Closed

Separate physical and logical layer into different crates #81

aannleax opened this issue Jan 4, 2023 · 4 comments
Labels
crate crate related Issue enhancement New feature or request logical logical layer physical physical layer

Comments

@aannleax
Copy link
Member

aannleax commented Jan 4, 2023

To have a clearer separation of the logical and physical layer we want to put the physical layer into its own crate which is then used by the logical layer.

The only problem that needs solving here is code that is currently used by both layers. This is:

  • meta/timing.rs
  • meta/logging.rs

The logging dependency could be solved by pulling the log statements out of this global file to the places where it used. The original reason for having them in a separate file was to reduce clutter in the functions which do work. The redesign has to keep this is mind. One way of doing this could be to use Display implementations to avoid verbose setup code.

This should wait until #70 is done since we currently have other dependencies relating to the type system right now.

@aannleax
Copy link
Member Author

aannleax commented Jan 4, 2023

Re logging: Another approach would be to simply divide the logging file into one for each layer.

To remedy potential performance implications of having those function calls, one could turn those into macros

@mmarx
Copy link
Member

mmarx commented Jan 4, 2023

As mentioned earlier today, logging needs to happen without any indirection to ensure that compile-time filtered log levels are properly elided without overhead. Using macros and/or #inline[always] will increase the size of the produced binary (which means less relevant code will fit in the instruction cache), while still not guaranteeing proper elision.

It's also not very ergonomic from a development perspective: All of the functions in feature/70-untangle/src/meta/logging.rs are called exactly once, so we're not saving on code duplication, but we're losing locality and context:

log_apply_rule(&self.program, self.current_step, current_rule_index);

tells me almost nothing about what is getting logged, whereas

log::info!("<<< {step}: APPLYING RULE {rule_index} >>>");

makes it obvious that we're logging the step number and the rule index (but not the actual rule). It's also less clutter (e.g., in terms of characters) than having the function call with the redundant program argument. Also, when changing log messages, there's now potentially two places that need to be modified, instead of a single place, and they're spread far apart in the code base, which will make code reviews touching either part harder.
log_load_table should rather be three distinct log messages at the appropriate match arms of the code, rather than doing another match on the data source type (and introducing another place that needs changing whenever a data source is added).
log_avaiable_variable_order [sic] should likely just be a Display implementation for RuleAnalysis.
log_fragmentation_combine produces different log levels depending on whether the given trie is None, this should definitely be two distinct log calls instead.
All other functions are single-line log calls, which can readily be moved back to where they belong.

@aannleax
Copy link
Member Author

aannleax commented Jan 5, 2023

Created a new issue for logging (#88)

Tbh I dont think the disadvantages are that severe. Performance doesnt matter here (and I dont want to believe that Rust is too stupid to optimize this properly) and knowing what exactly is being logged is irrelevant for someone who wants to understand what a function does. It just should take as little space as possible (ideally just one line)

But I dont really care either way, so if somebody wants to change this, I'm fine it

@mmarx mmarx added enhancement New feature or request crate crate related Issue physical physical layer logical logical layer labels Apr 26, 2023
@mmarx mmarx added this to nemo Apr 26, 2023
@github-project-automation github-project-automation bot moved this to Todo in nemo Apr 26, 2023
@mmarx mmarx added this to the Beyond the first release milestone Apr 26, 2023
@mmarx
Copy link
Member

mmarx commented Jun 23, 2023

This landed as part of #250.

@mmarx mmarx closed this as completed Jun 23, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in nemo Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate crate related Issue enhancement New feature or request logical logical layer physical physical layer
Projects
Archived in project
Development

No branches or pull requests

2 participants