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

Refactor internal implementation of rules #607

Open
tsipinakis opened this issue Feb 23, 2019 · 2 comments
Open

Refactor internal implementation of rules #607

tsipinakis opened this issue Feb 23, 2019 · 2 comments
Labels

Comments

@tsipinakis
Copy link
Member

tsipinakis commented Feb 23, 2019

Currently we're storing the rules in a huge struct. This is obviously neither space nor time efficient as we store a full value for all unset rules and on match we have to compare all possible match and set rules.

My proposed implementation would be something like this: (pseudocode, don't expect it to compile! :) )

enum rules_match {
    MATCH_APPNAME,
    ...
};
enum rules_set {
    SET_FORMAT,
    ...
};

struct rule_part {
    union {
        enum rules_match match;
        enum rules_set set;
    };
    void *data;
};

struct rule {
    struct rule_part *match_rules;
    struct rule_part *set_rules;
};

So as it's apparent this involves having each rule item in an enum, and inside the rule struct we have 2 arrays one for the match rules and one for the set rule.

In order to apply the rule we simply iterate over the match array and check if each rule_part matches correctly. If it does we start iterating on the set rules and applying each one appropriately.

The 2 big downsides are:

  • We have to go through each rule twice, one to count the number of match and set rules in order to create the array and one got actually creating it.
    This may be remedied by replacing the array with a linked list but that may be overcomplicating it.

  • Given Cs quite stingy approach with types we (unfortunately) have to abuse the void pointer again which leads to a big number of tiny allocations which might not be the best thing
    An alternative here might be to replace the void pointer with a union of every possible data type.

@tsipinakis tsipinakis changed the title Refactor rules Refactor internal implementation of rules Feb 23, 2019
@bebehei
Copy link
Member

bebehei commented Feb 25, 2019

The approach seems tempting. Running two times over the list might be a downside, but I'd gladly accept it if it allows abstraction of our rule.

Especially this lump of code would vanish

dunst/src/rules.c

Lines 93 to 104 in 7b3d85a

bool rule_matches_notification(struct rule *r, struct notification *n)
{
return (r->msg_urgency == URG_NONE || r->msg_urgency == n->urgency)
&& (r->match_transient == -1 || (r->match_transient == n->transient))
&& rule_field_matches_string(n->appname, r->appname)
&& rule_field_matches_string(n->desktop_entry, r->desktop_entry)
&& rule_field_matches_string(n->summary, r->summary)
&& rule_field_matches_string(n->body, r->body)
&& rule_field_matches_string(n->iconname, r->icon)
&& rule_field_matches_string(n->category, r->category)
&& rule_field_matches_string(n->stack_tag, r->stack_tag);
}

and would get replaced with a simple for loop:

bool rule_matches_notification(struct rule *r, struct notification *n)
{
        for (struct rulepart **r->match_rules; *rp; rp++)
                if (!rule_part_matches(n, *rp)
                        return false;
        return true;
}

Sexy.


But, how do you solve rule_part_matches?

That has to be a super huge switch case fuckup:

bool rule_part_matches(struct rule_part rp, notification *n)
{
        switch (rp->match) {
        case MATCH_APPNAME:
                return fnmatch(n->appname, rp->data...);
                break;
        case MATCH_...:
                return fnmatch(n->..., rp->data...);
                break;
        default:
                LOG_C("Invalid enum");
        }
}

Complexity just moved to another function. But it might be better to have a switch case instead of a hard wired list.


But wait, really? Keep in mind, that this is an invalid rule:

[rulename]
msg_urgency = low
msg_urgency = wrong
timeout = 10

It's forbidden and useless to have two values to apply to the same field. One value has to win this field.

A switch/case statement would help, processing multiple fields. But we won't ever have this.

@bebehei
Copy link
Member

bebehei commented Feb 25, 2019

Another note on the data space:

Also keep in mind, that an enum is usually 8 bytes long (as long as a pointer, probably to make switching easier).

This makes the new rule struct at least 16 bytes long with another 16 bytes per match/set declaration.

For myself, I've got on average 3 lines per rule in my config. Without the actual data, the structs take (16Byte + 3*16Byte) = 64 Bytes

In comparison to 168 Bytes of a current rule struct, that's 104 bytes on average shave off.

I should reiterate the advantages of the current approach:

  • It's typesafe.
  • It's faster.
  • Single memory chunk per rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants