Skip to content
This repository has been archived by the owner on Dec 18, 2017. It is now read-only.

Low hanging fruit performance improvements from profiling session #1167

Closed
wants to merge 5 commits into from
Closed

Low hanging fruit performance improvements from profiling session #1167

wants to merge 5 commits into from

Conversation

Praburaj
Copy link
Contributor

@Praburaj Praburaj commented Feb 4, 2015

This PR addresses the below bullet from this bug: #1147

Don't use Tracing system for logging, use Console.WriteLine for now. Trace loads the entire configuration stack.

@davidfowl

@ghost ghost added the cla-required label Feb 4, 2015
@@ -443,7 +443,7 @@ private static IEnumerable<string> GetSourcePatternCore(JObject rawProject, stri
}

// Assume it's an array (it should explode if it's not)
return token.ToObject<string[]>().SelectMany(GetSourcesSplit);
return token.Select(t => t.Value<string>()).SelectMany(GetSourcesSplit);
Copy link
Member

Choose a reason for hiding this comment

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

Lets add an extension method, ValueAsArray:

script.Value.Select(s => s.Value<string>()).ToArray()
script.ValueAsArray<string>()

You can have an overload that takes a property name as well:

foo["something"]?.ValueAsArray<string>()

Copy link
Member

Choose a reason for hiding this comment

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

And use it everywhere there's a ToObject<string[]>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll update the PR with this feedback.

@ghost ghost added cla-not-required and removed cla-required labels Feb 5, 2015
Praburaj added 4 commits February 5, 2015 09:53
This PR addresses the below bullet from this bug: #1147

Don't use Tracing system for logging, use Console.WriteLine for now. Trace loads the entire configuration stack.
This is to address the following bullet on bug : #1147

Don't use ToObject when parsing global/project.json, that causes the data contract serialization stack to load in JSON.NET which isn't required at all.
@Praburaj
Copy link
Contributor Author

Praburaj commented Feb 5, 2015

@davidfowl Updated PR.

globalSettings.ProjectSearchPaths = projectSearchPaths == null ? new string[] { } : projectSearchPaths.ToObject<string[]>();
globalSettings.ProjectSearchPaths = projectSearchPaths == null ?
new string[] { } :
projectSearchPaths.Select(p => p.Value<string>()).ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

ValueAsArray()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah.. fixed.

@Praburaj
Copy link
Contributor Author

Praburaj commented Feb 6, 2015

/CC @muratg

{
public static T[] ValueAsArray<T>(this JToken jToken)
{
return jToken.Select(a => a.Value<T>()).ToArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

need a null check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All callers handle the null here. But it wouldn't hurt adding a null check.

@muratg
Copy link
Contributor

muratg commented Feb 6, 2015

:shipit:

1 similar comment
@davidfowl
Copy link
Member

:shipit:

@Praburaj
Copy link
Contributor Author

Praburaj commented Feb 6, 2015

merged : f748be5

@Praburaj Praburaj closed this Feb 6, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants