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

[SPARK-31967][UI] Downgrade to vis.js 4.21.0 to fix Jobs UI loading time regression #28806

Closed
wants to merge 2 commits into from

Conversation

gengliangwang
Copy link
Member

@gengliangwang gengliangwang commented Jun 12, 2020

What changes were proposed in this pull request?

After #28192, the job list page becomes very slow.
For example, after the following operation, the UI loading can take >40 sec.

(1 to 1000).foreach(_ => sc.parallelize(1 to 10).collect) 

This is caused by a performance issue of vis-timeline. The serious issue affects both branch-3.0 and branch-2.4

I tried a different version 4.21.0 from https://cdnjs.com/libraries/vis
The infinite drawing issue seems also fixed if the zoom is disabled as default.

Why are the changes needed?

Fix the serious perf issue in web UI by falling back vis-timeline-graph2d to an ealier version.

Does this PR introduce any user-facing change?

Yes, fix the UI perf regression

How was this patch tested?

Manual test

@gengliangwang
Copy link
Member Author

I didn't find vis-timeline-graph2d.min.css.map and vis-timeline-graph2d.min.js.map in https://cdnjs.com/libraries/vis, so I deleted them. It should be minor.

@sarutak
Copy link
Member

sarutak commented Jun 12, 2020

I can't download the downgraded version of vis from https://cdnjs.com/libraries/vis.
Is it temporarily not available?
app-error

@gengliangwang
Copy link
Member Author

@sarutak it works fine in my side:
image

@sarutak
Copy link
Member

sarutak commented Jun 12, 2020

I can't still get from the URL but can get from https://almende.github.io/vis/ anyway.

@gengliangwang
Copy link
Member Author

OK, the 4.21.0 package in https://almende.github.io/vis/ should be the same

@sarutak
Copy link
Member

sarutak commented Jun 12, 2020

@gengliangwang I checked out the change and build but timeline-view wouldn't work.
timeline-constructor-error

I also tried replacing the library with the one from https://almende.github.io/vis/ and works.

@SparkQA
Copy link

SparkQA commented Jun 12, 2020

Test build #123889 has finished for PR 28806 at commit 53ac823.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gengliangwang
Copy link
Member Author

@sarutak ok I have replaced with the one from https://almende.github.io/vis/
Thanks for trying!

@sarutak
Copy link
Member

sarutak commented Jun 12, 2020

The infinite drawing issue seems also fixed if the zoom is disabled as default.

Actually that is not fixed in 4.21.0.
I replaced vis included in this PR with the one from https://almende.github.io/vis/ and I reproduced the infinite rendering.

infinite-rendering

Maybe, we compromise either infinite redrawing issue or this performance issue...

@gengliangwang
Copy link
Member Author

well, in my setup, if the "Enable zooming" checkbox is not clicked, the infinite redrawing won't happen.

Maybe, we compromise either infinite redrawing issue or this performance issue...

yeah, I prefer infinite redrawing to the 40 seconds perf issue ...

@sarutak
Copy link
Member

sarutak commented Jun 12, 2020

LGTM.
The updated version seems to work fine.
Pending for Jenkins.

@SparkQA
Copy link

SparkQA commented Jun 12, 2020

Test build #123900 has finished for PR 28806 at commit f90f2b4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sarutak
Copy link
Member

sarutak commented Jun 12, 2020

@gengliangwang Do you want to wait for other reviews?

@srowen
Copy link
Member

srowen commented Jun 12, 2020

Just for the record - this is a downgrade, upgrade?

@gengliangwang
Copy link
Member Author

Just for the record - this is a downgrade, upgrade?

@srowen it's downgrade

@gengliangwang
Copy link
Member Author

I am going to merge this one, and backport it to 3.0 / 2.4
@srowen @dongjoon-hyun @dbtsai @holdenk let me know if you have other concerns.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jun 12, 2020

Hi, @gengliangwang and @sarutak . I'm wondering if this is safer enough than reverting SPARK-31420. Are we sure that there is no more UI regressions? It's difficult to detect manually this kind of slowness regression.

cc @rxin and @holdenk since they are the recent release managers.
Also, cc @gatorsmile , @cloud-fan , @dbtsai

@gengliangwang
Copy link
Member Author

@dongjoon-hyun the PR #28192 is to fix the infinite timeline redraw bug in https://issues.apache.org/jira/browse/SPARK-31420 . The bug will happen again if we simply reverting #28192 .

This downgraded solution is better. In my local setup, the perf issue can't not be reproduced, and by default the infinite timeline redrawing won't happen unless the "Enable zooming" checkbox. I believe this is a better solution.

@gengliangwang
Copy link
Member Author

Talked to @dongjoon-hyun offline. He is ok to merge this now

@gengliangwang
Copy link
Member Author

Merging to master.

@gengliangwang gengliangwang changed the title [SPARK-31967][UI] UI Perf regression: Loading jobs UI page takes 40 seconds [SPARK-31967][UI] Downgrade to vis.js 4.21.0 to fix Jobs UI loading time regression Jun 13, 2020
@sarutak
Copy link
Member

sarutak commented Jun 13, 2020

This downgraded solution is better. In my local setup, the perf issue can't not be reproduced, and by default the infinite timeline redrawing won't happen unless the "Enable zooming" checkbox. I believe this is a better solution.

Downgrading vis.js is better though, actually, infinite redrawing can happen even if Enable zooming is not checked.
#28806 (comment) .
It seems to depend on browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants