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 WidgetUtils.WrapText performance on long lines. #14167

Merged
merged 1 commit into from Oct 25, 2017

Conversation

Projects
None yet
4 participants
@pchote
Member

pchote commented Oct 12, 2017

Fixes the performance cliff that caused the freeze in #14141.

The word wrap algorithm was scanning backwards from the end of the string which is horrific for performance when the line of text is hundreds of times longer than the wrap width. Scanning forwards instead places an upper bound on the number of iterations it needs to do before it terminates, fixing the issue and improving perf in the less extreme cases.

Testcase: paste a block of arbitrary text into the chat with and without the change: the results should be the same.

@pchote pchote added this to the Next + 1 milestone Oct 12, 2017

@pchote pchote requested a review from chrisforbes Oct 12, 2017

start = spaceIndex - 1;
m = font.Measure(line.Substring(0, spaceIndex));
var fragmentWidth = font.Measure(line.Substring(0, spaceIndex)).X;

This comment has been minimized.

@RoosterDragon

RoosterDragon Oct 13, 2017

Member

This still has quadratic behaviour, you could maintain the fragment length so far and just compute the increments to get back to linear time.

@RoosterDragon

RoosterDragon Oct 13, 2017

Member

This still has quadratic behaviour, you could maintain the fragment length so far and just compute the increments to get back to linear time.

This comment has been minimized.

@pchote

pchote Oct 13, 2017

Member

I wanted to avoid a potenial can of worms with kerning and ligatures – i'm not sure whether they interact with inter-word spaces, but it seems plausible that they could. The current approach is indeed still quadratic, but the term being squared is now bounded to ≲ 20 so it isn't really a problem anymore.

@pchote

pchote Oct 13, 2017

Member

I wanted to avoid a potenial can of worms with kerning and ligatures – i'm not sure whether they interact with inter-word spaces, but it seems plausible that they could. The current approach is indeed still quadratic, but the term being squared is now bounded to ≲ 20 so it isn't really a problem anymore.

This comment has been minimized.

@RoosterDragon

RoosterDragon Oct 13, 2017

Member

Given that Measure just blindly sums up characters widths, I think that's a can of worms we already opened and are wilfully ignoring (unless there's some magic I'm missing).

@RoosterDragon

RoosterDragon Oct 13, 2017

Member

Given that Measure just blindly sums up characters widths, I think that's a can of worms we already opened and are wilfully ignoring (unless there's some magic I'm missing).

This comment has been minimized.

@pchote

pchote Oct 13, 2017

Member

I'd like to think that we could fix that one day, but I guess we can always undo the change here if that day ever comes.

@pchote

pchote Oct 13, 2017

Member

I'd like to think that we could fix that one day, but I guess we can always undo the change here if that day ever comes.

This comment has been minimized.

@pchote

pchote Oct 15, 2017

Member

This still doesn't seem to work. Consider the following test case:

var font = Game.Renderer.Fonts["Regular"];
var testString = "Pellentesque dapibus interdum odio, ut rhoncus arcu malesuada non. Fusce bibendum, tortor";
var testFragments = new[] {
	"Pellentesque ",
	"dapibus ",
	"interdum ",
	"odio, ",
	"ut ",
	"rhoncus ",
	"arcu ",
	"malesuada ",
	"non. ",
	"Fusce ",
	"bibendum, ",
	"tortor"
};

Console.WriteLine("single: {0} aggregate: {1}", font.Measure(testString).X, testFragments.Select(x => font.Measure(x).X).Sum());

single: 588 aggregate: 591

Am I missing something obvious?

@pchote

pchote Oct 15, 2017

Member

This still doesn't seem to work. Consider the following test case:

var font = Game.Renderer.Fonts["Regular"];
var testString = "Pellentesque dapibus interdum odio, ut rhoncus arcu malesuada non. Fusce bibendum, tortor";
var testFragments = new[] {
	"Pellentesque ",
	"dapibus ",
	"interdum ",
	"odio, ",
	"ut ",
	"rhoncus ",
	"arcu ",
	"malesuada ",
	"non. ",
	"Fusce ",
	"bibendum, ",
	"tortor"
};

Console.WriteLine("single: {0} aggregate: {1}", font.Measure(testString).X, testFragments.Select(x => font.Measure(x).X).Sum());

single: 588 aggregate: 591

Am I missing something obvious?

This comment has been minimized.

@RoosterDragon

RoosterDragon Oct 18, 2017

Member

single: 592 aggregate: 592

Works on my machine? Happy to defer this to another time though.

@RoosterDragon

RoosterDragon Oct 18, 2017

Member

single: 592 aggregate: 592

Works on my machine? Happy to defer this to another time though.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 18, 2017

Member

Removing the Changes Requested label as it isn't clear whether those changes are feasible (see discussion above), and in any case could be deferred to a followup PR.

Member

pchote commented Oct 18, 2017

Removing the Changes Requested label as it isn't clear whether those changes are feasible (see discussion above), and in any case could be deferred to a followup PR.

@RoosterDragon

👍

@SmittyShitDevs

This comment has been minimized.

Show comment
Hide comment
@SmittyShitDevs

SmittyShitDevs Oct 20, 2017

Hey CatGirls420 here. Just letting you know I successfully built the new bleed release and the patch for the anti-spam works. I tested it and could no longer freeze/crash/lag the game by spamming ridiculously large messages (Over 4,000 characters, but i may be underestimating).

So pchote, you're solution seems to have worked. My tests we're successful.

SmittyShitDevs commented Oct 20, 2017

Hey CatGirls420 here. Just letting you know I successfully built the new bleed release and the patch for the anti-spam works. I tested it and could no longer freeze/crash/lag the game by spamming ridiculously large messages (Over 4,000 characters, but i may be underestimating).

So pchote, you're solution seems to have worked. My tests we're successful.

@reaperrr

Can confirm the performance improvement and encountered no issues.

@reaperrr reaperrr merged commit 77a7453 into OpenRA:bleed Oct 25, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pchote pchote deleted the pchote:wordwrap branch Apr 28, 2018

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