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

Stabilize V2 engine UI #6761

Merged
merged 5 commits into from Nov 15, 2018

Conversation

Projects
None yet
2 participants
@blorente
Contributor

blorente commented Nov 12, 2018

Problem

Described in #6666, the heavy-hitters displayed by the engine UI displayed at unstable locations.

Solution

Keep an IndexMap that maps Display "workers" to engine "tasks", and doesn't move tasks around when they are added.

Result

The engine UI looks more stable, and is easier to read.

Show resolved Hide resolved src/rust/engine/src/scheduler.rs Outdated
Show resolved Hide resolved src/rust/engine/src/scheduler.rs Outdated
Show resolved Hide resolved src/rust/engine/src/scheduler.rs Outdated
Show resolved Hide resolved src/rust/engine/src/scheduler.rs Outdated

@blorente blorente force-pushed the blorente:blorente/6666/improve-engine-ui branch from 466c41f to 8420f12 Nov 13, 2018

tasks_to_display.extend(heavy_hitters.iter().cloned());
// And remove the tasks that no longer should be there.
for (task, duration) in tasks_to_display.clone().into_iter() {
if !heavy_hitters.contains(&(task.to_string(), duration)) {

This comment has been minimized.

@stuhood

stuhood Nov 14, 2018

Member

On every run, the duration of a task will change... so I'm not sure that this is doing the right thing. It's possible that it should only be comparing the task name?

This might also be n^2 (small N, of course, but) because Vec::contains will be linear.

This comment has been minimized.

@blorente

blorente Nov 14, 2018

Contributor

Making heavy_hitters into a HashMap solved both problems (constant lookup, lookup by key). Since we don't actually care about the order of heavy_hitters anymore, it should be fine.

@stuhood

Thanks! Will merge on green.

@stuhood

This comment has been minimized.

Member

stuhood commented Nov 14, 2018

It looks like the test failure is due to #6661, but will see if @illicitonion has a chance to look at that before merging this on top.

@stuhood

This comment has been minimized.

Member

stuhood commented Nov 15, 2018

The TTY issue is now fixed in master. Merging.

@stuhood stuhood merged commit 550e4cd into pantsbuild:master Nov 15, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment