-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
[SPARK-4266] [Web-UI] Reduce stage page load time. #3328
Conversation
Test build #23512 has started for PR 3328 at commit
|
Test build #23512 has finished for PR 3328 at commit
|
Test PASSed. |
listingTableClass += " table-fixed" | ||
var listingTableClass = { | ||
if (stripeRowsWithCss) TABLE_CLASS_STRIPED | ||
else TABLE_CLASS_NOT_STRIPED | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do
var listingTableClass =
if (stripeRowsWithCss) {
TABLE_CLASS_STRIPED
} else {
TABLE_CLASS_NOT_STRIPED
}
Thanks Andrew! On further inspection I realized this fits in one line which is, I think, the cleanest way to do this. |
Test build #23618 has started for PR 3328 at commit
|
Test build #23618 has finished for PR 3328 at commit
|
Test FAILed. |
e2d2bb1
to
9470b4f
Compare
Test build #23629 has started for PR 3328 at commit
|
Test build #23629 has finished for PR 3328 at commit
|
Test FAILed. |
retest this please |
Test build #23647 has started for PR 3328 at commit
|
Test build #23647 has finished for PR 3328 at commit
|
Test PASSed. |
@andrewor14 @pwendell any more comments here? |
LGTM |
Yeah, LGTMT. Feel free to merge it |
* An ID selector (rather than a class selector) is used to ensure this runs quickly even on pages | ||
* with thousands of task rows (ID selectors are much faster than class selectors). */ | ||
function stripeSummaryTable() { | ||
$("#task-summary-table").find("tr:not(:hidden)").each(function (index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering - is using this selector more expensive than just traversing elements in children()
and checking manually for each one and then maintaining your own counter? It seems like interpreting this string could be expensive inside of javascript.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a table that has at most 6 or 7 rows (it's the summary table for each metric) -- so I don't think that level of optimization matters (the most important thing here is the ID selector, so you don't need to traverse the task table, which can have infinitely many rows)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't follow that. Thanks for explaining.
The commit changes the java script used to show/hide additional metrics in order to reduce page load time. SPARK-4016 significantly increased page load time for the stage page when stages had a lot (thousands or tens of thousands) of tasks, due to the additional Javascript to hide some metrics by default and stripe the tables. This commit reduces page load time in two ways: (1) Now, all of the metrics that are hidden by default are hidden by setting "display: none;" using CSS for the page, rather than hiding them using javascript after the page loads. Without this change, for stages with thousands of tasks, there was a few second delay after page load, where first the additional metrics were shown, and then after a delay were hidden once the relevant JS finished running. (2) CSS is used to stripe all of the tables except for the summary table. The summary table needs javascript to do the striping because some rows are hidden, but the javascript striping is slower, which again resulted in a delay when it was used for the task table (where for a few seconds after page load, all of the rows in the task table would be white, while the browser finished running the JS to stripe the table).
9470b4f
to
f964091
Compare
Ok I rebased this and will let Jenkins test it one more time (since it was last tested 5 days ago and I know lots has been merged since then) -- then I'll merge it assuming all of the tests pass. |
Test build #23799 has started for PR 3328 at commit
|
Test build #23799 has finished for PR 3328 at commit
|
Test PASSed. |
The commit changes the java script used to show/hide additional metrics in order to reduce page load time. SPARK-4016 significantly increased page load time for the stage page when stages had a lot (thousands or tens of thousands) of tasks, due to the additional Javascript to hide some metrics by default and stripe the tables. This commit reduces page load time in two ways: (1) Now, all of the metrics that are hidden by default are hidden by setting "display: none;" using CSS for the page, rather than hiding them using javascript after the page loads. Without this change, for stages with thousands of tasks, there was a few second delay after page load, where first the additional metrics were shown, and then after a delay were hidden once the relevant JS finished running. (2) CSS is used to stripe all of the tables except for the summary table. The summary table needs javascript to do the striping because some rows are hidden, but the javascript striping is slower, which again resulted in a delay when it was used for the task table (where for a few seconds after page load, all of the rows in the task table would be white, while the browser finished running the JS to stripe the table). cc pwendell This change is intended to be backported to 1.2 to avoid a regression in UI performance when users run large jobs. Author: Kay Ousterhout <kayousterhout@gmail.com> Closes #3328 from kayousterhout/SPARK-4266 and squashes the following commits: f964091 [Kay Ousterhout] [SPARK-4266] [Web-UI] Reduce stage page load time. (cherry picked from commit d24d5bf) Signed-off-by: Kay Ousterhout <kayousterhout@gmail.com>
The commit changes the java script used to show/hide additional
metrics in order to reduce page load time. SPARK-4016 significantly
increased page load time for the stage page when stages had a lot
(thousands or tens of thousands) of tasks, due to the additional
Javascript to hide some metrics by default and stripe the tables.
This commit reduces page load time in two ways:
(1) Now, all of the metrics that are hidden by default are
hidden by setting "display: none;" using CSS for the page,
rather than hiding them using javascript after the page loads.
Without this change, for stages with thousands of tasks, there
was a few second delay after page load, where first the additional
metrics were shown, and then after a delay were hidden once the
relevant JS finished running.
(2) CSS is used to stripe all of the tables except for the summary
table. The summary table needs javascript to do the striping because
some rows are hidden, but the javascript striping is slower, which
again resulted in a delay when it was used for the task table (where
for a few seconds after page load, all of the rows in the task table
would be white, while the browser finished running the JS to stripe
the table).
cc @pwendell
This change is intended to be backported to 1.2 to avoid a regression in
UI performance when users run large jobs.