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

Support the implicit "-a" in find #132

Merged
merged 1 commit into from
Feb 7, 2018
Merged

Conversation

danw
Copy link
Collaborator

@danw danw commented Feb 6, 2018

The find man page says that "expression expression" is equivalent to "expression -a expression".

This reduces the number of shell commands we need to run during regen check of one of the internal android trees by ~10%, which reduced the time spent in regen checking by ~60%.

before: *kati*: shell time (regen): 5.842119 / 516
after:  *kati*: shell time (regen): 2.377083 / 462

The find man page says that "expression expression" is equivalent to
"expression -a expression".

This reduces the number of shell commands we need to run during regen
check of one of the internal android trees by ~10%, which reduced the
time spent in regen checking by ~60%.

before: *kati*: shell time (regen): 5.842119 / 516
after:  *kati*: shell time (regen): 2.377083 / 462

Change-Id: I177f37cd7e12625fb2dbfa371f3d79cb625c84fe
Copy link
Contributor

@shinh shinh left a comment

Choose a reason for hiding this comment

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

The speed up sounds great! IIRC there was no expr expr-style find which consume a lot of time. Perhaps this style was introduced in new code? Anyway, it could be worth considering to disallow find commands which cannot be handled by kati?

Please feel free to merge this as is. The patch itself looks good to me, but I'd prefer code which doesn't have extra check for the first token of <fact>.

if (!GetNextToken(&tok) || tok.empty())
return NULL;
} else {
if (tok != "-not" && tok != "\\!" && tok != "\\(" && tok != "-name" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking if the next token is a prefix of <fact> would work now but in future it can be broken when the definition of <fact> has changed. Maybe better to do something like the following? I'm not very sure if my code works, but I think we shouldn't need two checks for "-not", "\\!", etc.

diff --git a/find.cc b/find.cc
index 1b9e23f..18450d0 100644
--- a/find.cc
+++ b/find.cc
@@ -526,15 +526,20 @@ class FindCommandParser {
     while (true) {
       if (!GetNextToken(&tok))
         return NULL;
-      if (tok != "-and" && tok != "-a") {
-        UngetToken(tok);
-        return c.release();
+      bool has_and = false;
+      if (tok == "-and" || tok == "-a") {
+        has_and = true;
+        if (!GetNextToken(&tok) || tok.empty())
+          return NULL;
       }
-      if (!GetNextToken(&tok) || tok.empty())
-        return NULL;
       unique_ptr<FindCond> r(ParseFact(tok));
       if (!r.get()) {
-        return NULL;
+        if (has_and) {
+          return NULL;
+        } else {
+          UngetToken(tok);
+          return c.release();
+        }
       }
       c.reset(new AndCond(c.release(), r.release()));
     }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had tried something similar, but ParseFact doesn't differentiate between an error and the token not being a fact. And that was necessary in order to parse things properly.

@danw
Copy link
Collaborator Author

danw commented Feb 7, 2018

The most expensive case (~1.5 seconds on it's own) was new. I'm looking into exposing the unsupported find cases as warnings, with an option to turn them into errors. We've still got a bunch of these though, so I need to figure out whether to fix Kati or fix the source.

I've already fixed the 6 instances that used | sort, which I could have implemented in Kati, but since we only had 6 instances, I just fixed the calls.

@danw danw merged commit 054627c into google:master Feb 7, 2018
@danw danw deleted the find_implicit_and branch February 7, 2018 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants