You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/08/25 05:15:00 UTC
[GitHub] [airflow] eejbyfeldt opened a new pull request #17821: Improve graph view load time for dags with open groups
eejbyfeldt opened a new pull request #17821:
URL: https://github.com/apache/airflow/pull/17821
The main improvement is calling the expensive draw function only one
time during the initial load instead of for each saved group.
The previous behavior could cause significat slowness for when loading
the graph view for large dags with open task groups.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton merged pull request #17821: Improve graph view load time for dags with open groups
Posted by GitBox <gi...@apache.org>.
ryanahamilton merged pull request #17821:
URL: https://github.com/apache/airflow/pull/17821
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi edited a comment on pull request #17821: Improve graph view load time for dags with open groups
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #17821:
URL: https://github.com/apache/airflow/pull/17821#issuecomment-906248066
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eejbyfeldt commented on pull request #17821: Improve graph view load time for dags with open groups
Posted by GitBox <gi...@apache.org>.
eejbyfeldt commented on pull request #17821:
URL: https://github.com/apache/airflow/pull/17821#issuecomment-905421759
> Nice work! Could you provide steps on how to test the performance locally?
Yes, here is what i did:
In order to run the webserver locally I did the following.
Build and install airflow in a venv
```
$ python -m venv .venv
$ source .venv/bin/activate
$ pip install .
```
initialize the db and add a user
```
$ airflow db init
$ airflow users create \
--username admin \
--role Admin \
--firstname test \
--lastname airflow \
--email '' \
--password admin
```
and compile the static assets in airflow (requires yarn and nodejs)
```
$ airflow/www/compile_assets.sh
```
Then I stated the webserver locally
```
$ airflow webserver
```
The used a browser and went to `localhost:8080` and opened the example dag of `example_task_group` to get a dag with some task groups. This dag is not really large enough where it is really slow, but the extra calls to `draw` and increase render time show up in the performance tab of the developer tools in Firefox (I am sure other browsers have similar features). I mostly used the flame chart (https://developer.mozilla.org/en-US/docs/Tools/Performance/Flame_Chart) to discover what caused the slowness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eejbyfeldt commented on a change in pull request #17821: Improve graph view load time for dags with open groups
Posted by GitBox <gi...@apache.org>.
eejbyfeldt commented on a change in pull request #17821:
URL: https://github.com/apache/airflow/pull/17821#discussion_r695652502
##########
File path: airflow/www/static/js/graph.js
##########
@@ -598,7 +601,7 @@ function focusGroup(nodeId) {
}
// Expands a group node
-function expandGroup(nodeId, node, focus = true) {
+function expandGroup(nodeId, node, focus = true, do_draw = true) {
Review comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #17821: Improve graph view load time for dags with open groups
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #17821:
URL: https://github.com/apache/airflow/pull/17821#issuecomment-906248066
Oh I'm sorry to make you write all that up I guess I was just trying to find an example where it was noticaebly slower to render.
This looks good but I think we can clean it up a little more. There is only one time we call `expandGroup()` (line 153) and want to focus and draw. Instead of carrying around these variables and calling `draw` and `focusGroup` inside of `expandGroup` we just call those two functions immediately after expandGroup in that one situation. Like so
```
expandGroup(nodeId, node);
draw();
focusGroup(nodeId);```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi edited a comment on pull request #17821: Improve graph view load time for dags with open groups
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #17821:
URL: https://github.com/apache/airflow/pull/17821#issuecomment-906248066
Oh I'm sorry to make you write all that up I guess I was just trying to find an example where it was noticaebly slower to render.
This looks good but I think we can clean it up a little more. There is only one time we call `expandGroup()` (line 153) and want to focus and draw. Instead of carrying around these variables and calling `draw` and `focusGroup` inside of `expandGroup` we just call those two functions immediately after expandGroup in that one situation. Like so
```
expandGroup(nodeId, node);
draw();
focusGroup(nodeId);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eejbyfeldt commented on pull request #17821: Improve graph view load time for dags with open groups
Posted by GitBox <gi...@apache.org>.
eejbyfeldt commented on pull request #17821:
URL: https://github.com/apache/airflow/pull/17821#issuecomment-906265245
> This looks good but I think we can clean it up a little more. There is only one time we call `expandGroup()` ([line 153](https://github.com/apache/airflow/blob/37b39f40361ba0ce6c0a5b209d2a2e05d9824f37/airflow/www/static/js/graph.js#L153)) and want to focus and draw. Instead of carrying around these variables and calling `draw` and `focusGroup` inside of `expandGroup` we just call those two functions immediately after expandGroup in that one situation. Like so
Good suggestion, fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi edited a comment on pull request #17821: Improve graph view load time for dags with open groups
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #17821:
URL: https://github.com/apache/airflow/pull/17821#issuecomment-906248066
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #17821: Improve graph view load time for dags with open groups
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #17821:
URL: https://github.com/apache/airflow/pull/17821#discussion_r695575356
##########
File path: airflow/www/static/js/graph.js
##########
@@ -598,7 +601,7 @@ function focusGroup(nodeId) {
}
// Expands a group node
-function expandGroup(nodeId, node, focus = true) {
+function expandGroup(nodeId, node, focus = true, do_draw = true) {
Review comment:
Could we change the variable to `shouldDraw`? js should be camelCased and I'm not sure if `do` is a vague verb.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org