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 2022/04/07 15:34:59 UTC
[GitHub] [airflow] bbovenzi opened a new pull request, #22835: Remove extraneous json parsing in Calendar and Grid
bbovenzi opened a new pull request, #22835:
URL: https://github.com/apache/airflow/pull/22835
Both `/grid` and `/calendar` use `htmlsafe_json_dumps()` in the webserver before passing data to the UI. The data is already json and doesn't need to be parsed again. In fact, `{{ data }}` was already a json object while `{{ data|tojson }}` was converting it back to a string which required calling `JSON.parse({{ data|tojson }})`.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] ashb commented on a diff in pull request #22835: Remove extraneous json parsing in Calendar and Grid
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22835:
URL: https://github.com/apache/airflow/pull/22835#discussion_r845290649
##########
airflow/www/static/js/calendar.js:
##########
@@ -63,9 +63,6 @@ const dateFormat = 'YYYY-MM-DD';
document.addEventListener('DOMContentLoaded', () => {
$('span.status_square').tooltip({ html: true });
- // JSON.parse is faster for large payloads than an object literal
- const rootData = JSON.parse(calendarData);
Review Comment:
It's detailed in that PR. The point is that an object literal slows down DOMReady, so delaying it to after the event via `JSON.parse` makes it more performant. So yes, `const calendarData = {{ data|tojson }};` makes calendarData a string that. That's the point.)
(The variable is badly named.)
--
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 closed pull request #22835: Remove extraneous json parsing in Calendar and Grid
Posted by GitBox <gi...@apache.org>.
bbovenzi closed pull request #22835: Remove extraneous json parsing in Calendar and Grid
URL: https://github.com/apache/airflow/pull/22835
--
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] potiuk commented on a diff in pull request #22835: Remove extraneous json parsing in Calendar and Grid
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22835:
URL: https://github.com/apache/airflow/pull/22835#discussion_r845288649
##########
airflow/www/static/js/calendar.js:
##########
@@ -63,9 +63,6 @@ const dateFormat = 'YYYY-MM-DD';
document.addEventListener('DOMContentLoaded', () => {
$('span.status_square').tooltip({ html: true });
- // JSON.parse is faster for large payloads than an object literal
- const rootData = JSON.parse(calendarData);
Review Comment:
Explanation here: https://v8.dev/blog/cost-of-javascript-2019#json
--
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] ashb commented on a diff in pull request #22835: Remove extraneous json parsing in Calendar and Grid
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22835:
URL: https://github.com/apache/airflow/pull/22835#discussion_r845283231
##########
airflow/www/static/js/calendar.js:
##########
@@ -63,9 +63,6 @@ const dateFormat = 'YYYY-MM-DD';
document.addEventListener('DOMContentLoaded', () => {
$('span.status_square').tooltip({ html: true });
- // JSON.parse is faster for large payloads than an object literal
- const rootData = JSON.parse(calendarData);
Review Comment:
We want to keep this `JSON.parse` -- it's a _huge_ performance improvement for large DAGs.
--
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] ashb commented on a diff in pull request #22835: Remove extraneous json parsing in Calendar and Grid
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22835:
URL: https://github.com/apache/airflow/pull/22835#discussion_r845284032
##########
airflow/www/static/js/calendar.js:
##########
@@ -63,9 +63,6 @@ const dateFormat = 'YYYY-MM-DD';
document.addEventListener('DOMContentLoaded', () => {
$('span.status_square').tooltip({ html: true });
- // JSON.parse is faster for large payloads than an object literal
- const rootData = JSON.parse(calendarData);
Review Comment:
See #7492
--
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 diff in pull request #22835: Remove extraneous json parsing in Calendar and Grid
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #22835:
URL: https://github.com/apache/airflow/pull/22835#discussion_r845297135
##########
airflow/www/static/js/calendar.js:
##########
@@ -63,9 +63,6 @@ const dateFormat = 'YYYY-MM-DD';
document.addEventListener('DOMContentLoaded', () => {
$('span.status_square').tooltip({ html: true });
- // JSON.parse is faster for large payloads than an object literal
- const rootData = JSON.parse(calendarData);
Review Comment:
Ok I see. My bad. I guess I forgot that runtime is different than when jinja is parsing.
--
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 diff in pull request #22835: Remove extraneous json parsing in Calendar and Grid
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #22835:
URL: https://github.com/apache/airflow/pull/22835#discussion_r845286179
##########
airflow/www/static/js/calendar.js:
##########
@@ -63,9 +63,6 @@ const dateFormat = 'YYYY-MM-DD';
document.addEventListener('DOMContentLoaded', () => {
$('span.status_square').tooltip({ html: true });
- // JSON.parse is faster for large payloads than an object literal
- const rootData = JSON.parse(calendarData);
Review Comment:
How so? It is already a json object at this point... Or should we just not use `htmlsafe_json_dumps()`?
--
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] ashb commented on a diff in pull request #22835: Remove extraneous json parsing in Calendar and Grid
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22835:
URL: https://github.com/apache/airflow/pull/22835#discussion_r845294811
##########
airflow/www/static/js/calendar.js:
##########
@@ -63,9 +63,6 @@ const dateFormat = 'YYYY-MM-DD';
document.addEventListener('DOMContentLoaded', () => {
$('span.status_square').tooltip({ html: true });
- // JSON.parse is faster for large payloads than an object literal
- const rootData = JSON.parse(calendarData);
Review Comment:
(Thanks GitHub for not showing me Jarek's comments.)
--
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] ashb commented on a diff in pull request #22835: Remove extraneous json parsing in Calendar and Grid
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22835:
URL: https://github.com/apache/airflow/pull/22835#discussion_r845293558
##########
airflow/www/static/js/calendar.js:
##########
@@ -63,9 +63,6 @@ const dateFormat = 'YYYY-MM-DD';
document.addEventListener('DOMContentLoaded', () => {
$('span.status_square').tooltip({ html: true });
- // JSON.parse is faster for large payloads than an object literal
- const rootData = JSON.parse(calendarData);
Review Comment:
https://v8.dev/blog/cost-of-javascript-2019#json
--
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] potiuk commented on a diff in pull request #22835: Remove extraneous json parsing in Calendar and Grid
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22835:
URL: https://github.com/apache/airflow/pull/22835#discussion_r845288884
##########
airflow/www/static/js/calendar.js:
##########
@@ -63,9 +63,6 @@ const dateFormat = 'YYYY-MM-DD';
document.addEventListener('DOMContentLoaded', () => {
$('span.status_square').tooltip({ html: true });
- // JSON.parse is faster for large payloads than an object literal
- const rootData = JSON.parse(calendarData);
Review Comment:
And it makes sense
--
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