Skip to content
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

Be more efficient in how we look up watched WS paths. #260

Merged
merged 8 commits into from
May 14, 2019
Merged

Conversation

brian-brazil
Copy link
Contributor

1.5s -> 60ms.

@brian-brazil
Copy link
Contributor Author

Hold up, might be a bug here.

Processing many hundreds of XML elements
that we don't need takes hundreds of milliseconds.
This makes it easier to write handlers.
@brian-brazil
Copy link
Contributor Author

Okay, expanded the scope a bit. We're now going from ~20s to ~1.5s overall.

This brings us from ~20s to ~1.5s with 60ish Jams,
and is also generally more responsive.

Time the WS.Connect after all the registrations, to
avoid n^2 updates in the browser.

Use one big optimised handler over many tiny leaking handlers.
Cache element lookups.
For long lived screens with lots of churn, this could be an issue.
@brian-brazil
Copy link
Contributor Author

And PT loading with 90 penalties is now 5x faster. Creating penalties is already quite fast, so no need for changes there.

Overall effect is to make the JS ~5x faster to load the PT screen.

Don't adjust more than we have to on each update.
Be smarter on updating team penalty total.
Be smarter about how we search the DOM.
Copy link
Contributor

@frank-weinberg frank-weinberg left a comment

Choose a reason for hiding this comment

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

The period and jam tables in the operator screen could probably also profit from a rework using the amended WS keys.

return;
}
if (i >= p.length()) {
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition should be in the head of the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd result in us iterating once when we don't need to, which is why the head.exists check is before it.

PathTrie head = this;
for (int i = 0;; i++) {
if (head.exists) {
// Already covered.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment doesn't really make sense in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

break;

default:
if (k.parts[4] == 'ScoringTrip' && k.ScoringTrip >= 3 && k.ScoringTrip <= 10) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be <10.
Trip 10 is already part of the cell that's treated below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

scoreAfterSP = scoreAfterSP=='' ? tripScore : scoreAfterSP + "+" + tripScore;
} else {
scoreBeforeSP = scoreBeforeSP=='' ? tripScore : scoreBeforeSP + "+" + tripScore;
} else if (k.parts[4] == 'ScoringTrip' && k.ScoringTrip > 10) {
Copy link
Contributor

Choose a reason for hiding this comment

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

>= 10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (isTrue(WS.state[prefix+'ScoringTrip('+t+').AfterSP'])) {
scoreAfterSP = scoreAfterSP=='' ? tripScore : scoreAfterSP + "+" + tripScore;
} else {
scoreBeforeSP = scoreBeforeSP=='' ? tripScore : scoreBeforeSP + "+" + tripScore;
Copy link
Contributor

Choose a reason for hiding this comment

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

These need spaces around the + so the text will wrap when needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, though the wrapping means the SK tables no longer align in the (rare) case someone somehow manages to have 11+ scoring passes.

spRow.find('Trip10').text(scoreAfterSP);
});
jamRow.find('Trip10').text(scoreBeforeSP);
spRow.find('Trip10').text(scoreAfterSP);
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be .Trip10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

} else if (k.parts[4] == 'ScoringTrip' && k.ScoringTrip > 10) {
var scoreBeforeSP = '';
var scoreAfterSP = '';
$.each(new Array(3), function(idx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're now catching any trip index, this should be a while loop that ends, when a trip score is null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
}

function finalizePeriod(nr) {} //TODO: implement
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this? This is still an active todo on my list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's dead code currently, it can be added back when needed.

@@ -29,7 +29,11 @@ function preparePltInputTable(element, teamId, mode, statsbookPeriod, alternateN
$('<td>').text('Pivot').appendTo(thead);
$('<td>').text('Blocker').appendTo(thead);
$('<td>').text('Box').appendTo(thead);
$('<td>').appendTo(thead);
if (mode == 'lt') {
$('<td>').attr('id', 'head').text('Team ' + teamId).appendTo(thead);
Copy link
Contributor

Choose a reason for hiding this comment

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

This field is to small for a long team name.
I'd rather use it for the SP indicator (on PLT as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're probably right, but it'll do the job for now.

@brian-brazil
Copy link
Contributor Author

The period and jam tables in the operator screen could probably also profit from a rework using the amended WS keys.

They're actually a notable hotspot (though this PR helps), so I'll be reworking them at some point.

@frank-weinberg frank-weinberg merged commit 76bc323 into dev May 14, 2019
@brian-brazil brian-brazil deleted the ws-perf branch May 14, 2019 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants