Skip to content

Improve search aria label generation performance#34491

Merged
roblourens merged 1 commit into
microsoft:masterfrom
KuromiAK:acao/search-aria
Sep 19, 2017
Merged

Improve search aria label generation performance#34491
roblourens merged 1 commit into
microsoft:masterfrom
KuromiAK:acao/search-aria

Conversation

@KuromiAK

Copy link
Copy Markdown
Contributor

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

roblourens commented Sep 18, 2017

Copy link
Copy Markdown
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!

@KuromiAK

KuromiAK commented Sep 18, 2017

Copy link
Copy Markdown
Contributor Author

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 September 19, 2017 08:37
@sandy081 sandy081 removed their assignment Sep 19, 2017
@roblourens

Copy link
Copy Markdown
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.

@roblourens

Copy link
Copy Markdown
Member

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

@KuromiAK

Copy link
Copy Markdown
Contributor Author

I'll leave lcut to you.

@roblourens roblourens merged commit 3bb6de7 into microsoft:master Sep 19, 2017
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 27, 2020
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