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

improve test performance measuring #705

Merged
merged 1 commit into from
Jul 28, 2017

Conversation

radekdoulik
Copy link
Member

  • measure JNIEnv.Initialize end

  • added timing definitions file parameter to the ProcessLogcatTiming
    task. It contains data series labels and regex patterns to match
    logcat lines with timestamp, separated by semicolon. see
    build-tools/scripts/TimingDefinitions.txt as an example.

    if the pattern contains match named message, then the logger uses
    this match value instead of whole matched line (without timestamp)

@radekdoulik radekdoulik added the do-not-merge PR should not be merged. label Jul 27, 2017
@radekdoulik radekdoulik removed the do-not-merge PR should not be merged. label Jul 27, 2017
@radekdoulik radekdoulik requested a review from jonpryor July 27, 2017 18:14
@jonpryor
Copy link
Member

I love the idea. I have some ideas for the implementation. :-)


while ((line = reader.ReadLine ()) != null) {
int index = line.IndexOf (';');
if (index < 1 || index == line.Length)
Copy link
Member

Choose a reason for hiding this comment

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

You should support comments within the file, and probably treat any line starting with # as a comment character.

string line;

while ((line = reader.ReadLine ()) != null) {
int index = line.IndexOf (';');
Copy link
Member

Choose a reason for hiding this comment

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

Why'd you choose ; as the separator? I'd think that (space) or = (equal) would make a bit more sense:

# current
last;monodroid-timing:\s+(?<message>.*)$

# vs one of
last monodroid-timing:\s+(?<message>.*)$
last=monodroid-timing:\s+(?<message>.*)$

I think I find the more readable, but the = has a nice connotation of "this is the key with an associated regex."

int index = line.IndexOf (';');
if (index < 1 || index == line.Length)
continue;
var label = line.Substring (0, index);
Copy link
Member

Choose a reason for hiding this comment

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

You should check that label is a non-empty string, if only for a better error message and sanity. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

It already checks for it, if index < 1 then it skips to the next line

Log.LogWarning ($"unable to create regex for label: {label} from pattern: {pattern}\n{e}");
continue;
}
definedRegexs [line.Substring (0, index)] = regex;
Copy link
Member

Choose a reason for hiding this comment

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

This should instead be:

definedRegexs [label] = regex;

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, is this wise or desirable? This allows overwriting previous labels, e.g. with a file of:

a;some-regex
a;another-regex
a;do we have enough duplicates yet?

Is it really a good idea for the "last" one to replace anything previously defined, without even a warning?

 - measure JNIEnv.Initialize end

 - added timing definitions file parameter to the ProcessLogcatTiming
   task. It contains data series labels and regex patterns to match
   logcat lines with timestamp, separated by '=' character. see
   build-tools/scripts/TimingDefinitions.txt as an example. it can
   also contains comments, on the lines starting with '#' character

   if the pattern contains match named message, then the logger uses
   this match value instead of whole matched line (without timestamp)
@radekdoulik radekdoulik force-pushed the pr-perf-measuring-improvements branch from be8c53e to 62a9deb Compare July 28, 2017 09:10
@jonpryor jonpryor merged commit 6b83f34 into dotnet:master Jul 28, 2017
jonpryor pushed a commit that referenced this pull request Sep 9, 2020
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1193891

Changes: dotnet/java-interop@afbc5b3...ac914ce

  * dotnet/java-interop@ac914ce3: [ci] Add DevDiv required Roslyn analyzers, fix errors (#704)
  * dotnet/java-interop@a98c1ae1: [build] fix env vars causing gradle build error (#705)
  * dotnet/java-interop@75354b9f: [Java.Interop] Dispose, *then* Remove (#708)
  * dotnet/java-interop@8ea0bb28: [jnimarshalmethod-gen] Localizable errors (#696)

Commit dotnet/java-interop@ac914ce3 moved the location of
`NullableAttributes.cs`; update
`Xamarin.Android.Tools.JavadocImporter.csproj` appropriately.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants