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 search aria label generation performance #34491

Merged
merged 1 commit into from Sep 19, 2017

Conversation

Projects
None yet
3 participants
@KuromiAK
Contributor

KuromiAK commented Sep 15, 2017

When using search, if a matched line is really long (i.e. from a minified file), the UI freezes briefly because of a call to strings.lcut() which internally calls split(/\b/). This change removes the call with no change to behavior.

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 18, 2017

Member

#31551

I think that it's also due to the search tool producing large amounts of text and sending it all back from the search process. Also, it looks like we are still calling .preview for every match anyway? But the call you've replaced was definitely unnecessary so it looks like a good optimization. I'll try it out, thanks!

Member

roblourens commented Sep 18, 2017

#31551

I think that it's also due to the search tool producing large amounts of text and sending it all back from the search process. Also, it looks like we are still calling .preview for every match anyway? But the call you've replaced was definitely unnecessary so it looks like a good optimization. I'll try it out, thanks!

@KuromiAK

This comment has been minimized.

Show comment
Hide comment
@KuromiAK

KuromiAK Sep 18, 2017

Contributor

I did not remove the other preview call because it would alter the existing behavior. That said, I managed to rewrite the lcut function to reduce its memory footprint and speed it up quite a bit. It is also strange that lcut can return a string longer than n.

function lcut2(text, n) {
	if (text.length < n) {
		return text;
	}

	let re = /\b/g;
	let i = re.lastIndex;
	while (re.test(text)) {
		if (text.length - re.lastIndex < n) {
			break;
		}
		i = re.lastIndex;
		re.lastIndex += 1;
	}
	return text.substring(i).replace(/^\s/, '');
}
Contributor

KuromiAK commented Sep 18, 2017

I did not remove the other preview call because it would alter the existing behavior. That said, I managed to rewrite the lcut function to reduce its memory footprint and speed it up quite a bit. It is also strange that lcut can return a string longer than n.

function lcut2(text, n) {
	if (text.length < n) {
		return text;
	}

	let re = /\b/g;
	let i = re.lastIndex;
	while (re.test(text)) {
		if (text.length - re.lastIndex < n) {
			break;
		}
		i = re.lastIndex;
		re.lastIndex += 1;
	}
	return text.substring(i).replace(/^\s/, '');
}

@sandy081 sandy081 requested a review from roblourens Sep 19, 2017

@sandy081 sandy081 removed their assignment Sep 19, 2017

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 19, 2017

Member

The aria label change is a huge speedup! Nice find. The lcut change looks good too. Do you want to add it to the PR? I can just copy it in but I want to give you credit.

Member

roblourens commented Sep 19, 2017

The aria label change is a huge speedup! Nice find. The lcut change looks good too. Do you want to add it to the PR? I can just copy it in but I want to give you credit.

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 19, 2017

Member

We should have some unit tests for that function before we rewrite it.

Member

roblourens commented Sep 19, 2017

We should have some unit tests for that function before we rewrite it.

@KuromiAK

This comment has been minimized.

Show comment
Hide comment
@KuromiAK

KuromiAK Sep 19, 2017

Contributor

I'll leave lcut to you.

Contributor

KuromiAK commented Sep 19, 2017

I'll leave lcut to you.

@roblourens roblourens merged commit 3bb6de7 into Microsoft:master Sep 19, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
license/cla All CLA requirements met.

@roblourens roblourens referenced this pull request Sep 19, 2017

Closed

Optimize strings.lcut #34665

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment