Exit when json::parse_string failed, avoid double valid string check

[miyanyan]

There are two points to call json::parse_string

vcpkg-tool/src/vcpkg/base/json.cpp

Lines 910 to 928 in 46b8cce

	std::pair<std::string, Value> parse_kv_pair() noexcept
	{
	skip_whitespace();
	auto current = cur();
	std::pair<std::string, Value> res = {std::string(""), Value()};
	if (current == Unicode::end_of_file)
	{
	add_error(msg::format(msgUnexpectedEOFExpectedName));
	return res;
	}
	if (current != '"')
	{
	add_error(msg::format(msgUnexpectedCharExpectedName));
	return res;
	}
	res.first = parse_string();

vcpkg-tool/src/vcpkg/base/json.cpp

Lines 1025 to 1039 in 46b8cce

	Value parse_value() noexcept
	{
	skip_whitespace();
	char32_t current = cur();
	if (current == Unicode::end_of_file)
	{
	add_error(msg::format(msgUnexpectedEOFExpectedValue));
	return Value();
	}
	switch (current)
	{
	case '{': return parse_object();
	case '[': return parse_array();
	case '"': return Value::string(parse_string());

The current implementation of json::parse_string doesn't exit on invalid strings. So when the string is invalid:

• json::parse_value exits but performs redundant string validation
• json::parse_kv_pair continues processing even with an invalid string, which is unnecessary.

[BillyONeal]

This code is quite clearly trying to return the error message rather than terminate, I don't think elevating an error into termination is appropriate, particularly when this can happen from whatever user supplied input rather than a bug in the tool.

[miyanyan]

Currently there are only two functions call json::parse_string, when the string is invalid:

• json::parse_value -> Value::string -> json::parse_string, it will also terminate in Value::string
• json::parse_object -> json::parse_kv_pair -> json::parse_string, it will not terminate, but continue to create a invalid kv_pair in json::parse_kv_pair, and insert it in json::parse_object

I don't think elevating an error into termination is appropriate

I agree with this, but seems json::parse_value and json::parse_object don't behave the same

[BillyONeal]

Unless there is a fairly extreme perf cost from the check on 330 I'm not really seeing a good reason to add this API. If there is an extreme perf reason, perhaps we should consider altering how that assertion works rather than adding a new API footgun like this.

[miyanyan]

Yeah, it's not much necessary. I am investigating why sometimes vcpkg search is slow and then find this json::parse_value function

When I pref vcpkg search a, the json::parse_value function only costs about 5%-10%

[BillyONeal]

I'm guessing you're using manifest mode? In that case the reason search is awfully slow is that we end up launching a git process per port to extract the tree sha. I had some work in progress to use git mktree to replace N git invocations with 2 git invocations but I haven't had time to get back to that.

Also unfortunately child process launch time etc. won't be detected by a sampling based profiler like this since it only cares about CPU time in the process in question.

[miyanyan]

I'm using the classic mode, and most of the time vcpkg search takes only < 500ms, but occasionally it's slow, up to 5 seconds I guess

[BillyONeal]

Did you take your profile during one of the times where it was slow?

[miyanyan]

No, currently I can't reproduce when build vcpkg from source

"slow" only occured when using offical vcpkg in classic mode, and not occured on all of my PCs

[miyanyan]

I can reproduce when the first time I compile after I reboot, the search method costs about 5s

[BillyONeal]

Closed and reopened to force re-merge to pick up the macOS build fix