-
Notifications
You must be signed in to change notification settings - Fork 14.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
[AIRFLOW-6871] optimize tree view for large DAGs #7492
Conversation
83548cb
to
41c65e1
Compare
Codecov Report
@@ Coverage Diff @@
## master #7492 +/- ##
==========================================
+ Coverage 86.76% 86.85% +0.08%
==========================================
Files 896 896
Lines 42649 42663 +14
==========================================
+ Hits 37005 37055 +50
+ Misses 5644 5608 -36
Continue to review full report at Codecov.
|
41c65e1
to
84998e0
Compare
Can you expand on "seralize JSON as string to be used with JSON.parse on the client side |
84998e0
to
0e4e965
Compare
@ashb a quick analysis can be found at: https://developpaper.com/json-parse-is-faster-than-object-literal-syntax/. This optimization only makes sense for very large JSON payload, which is the case for tree.html. For the large DAGs that we have, it reduces page load time by couple hundred milliseconds. |
ebcd4e5
to
2697fda
Compare
2697fda
to
9912faa
Compare
035855f
to
67766d2
Compare
@ashb all builds are passing now, ready for another round of review. |
if task.depends_on_past: | ||
node['depends_on_past'] = task.depends_on_past | ||
if task.start_date: | ||
# round to seconds to reduce payload size |
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.
How much does it reduce it by? Is stripping of all ms noticable? (Could we perhaps limit to 3 or 6 sig. fig.?)
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.
not much for task node since we don't have too many of them, that's why i didn't add the rounding in the first place here. It did make a big difference for task instance node since we have lots of them, IIRC, probably around 10-20% size reduction.
I can change it to round to 3 sig. fig. everywhere to see what the performance implication would be.
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.
@ashb round to 3 sig. fig. increases the overall payload size by 15%. The question now is do we care about millisecond accuracy for task start/end time enough to take this 15% performance hit?
So far, I found second granularity has been good enough for us, but I might be missing other use-cases.
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.
Probably not needed.
Is it worth making it a config option do you think?
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 code path has a very hot function call loop that's very sensitive to if statements. For the large DAG that we have, adding one extra if statement increases the response time by more than 400ms. That's why simplify reduce_nodes() logic, remove unnecessary if statements
is in the optimization list :)
That and based on the understanding that we are rewriting Airflow web into a proper SPA, I think it's best not to introduce a config for this change. I would prefer us giving round to second a try and come back to add more sig. fig. or add a config later on if any real use-case comes up. It's better to not engineer solutions when we don't have a good use-case in mind.
If you are really concerned about the precision, we can perhaps change it to round to 1 sig. fig. for a 7% performance hit. I can't really think of a case where knowing 0.01 second difference of runtime is important.
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.
Good reasoning. I'll copy some/most of this in to the commit message for future-proofing.
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.
Leaving this unresolved as a reminder to myself.
67766d2
to
c47964a
Compare
c47964a
to
adf0c6d
Compare
916dc8e
to
cb933e3
Compare
This change reduces page size by more than 10X and reduces page load time by 3-5X. As a result, the tree view can now load large DAGs that were causing 5XX error without the patch. List of optimizations applied to the view handler: * only seralize used task instance attributes to json instead of the whole ORM object * encode task instance attributes as array instead of dict * encode datetime in unix timestamp instead of iso formmat string * push task instance attribute construction into client side JS * remove redundant task instance attributes * simplify reduce_nodes() logic, remove unnecessary if statements * seralize JSON as string to be used with JSON.parse on the client side to speed up browser JS parse time * remove spaces in seralized JSON string to reduce payload size
cb933e3
to
f57038e
Compare
@ashb updated if statement and CI is passing now :) |
This change reduces page size by more than 10X and reduces page load time by 3-5X. As a result, the tree view can now load large DAGs that were causing 5XX error without the patch. List of optimizations applied to the view handler: * only seralize used task instance attributes to json instead of the whole ORM object * encode task instance attributes as array instead of dict * encode datetime in unix timestamp instead of iso formmat string * push task instance attribute construction into client side JS * remove redundant task instance attributes * simplify reduce_nodes() logic, remove unnecessary if statements * seralize JSON as string to be used with JSON.parse on the client side to speed up browser JS parse time * remove spaces in seralized JSON string to reduce payload size
This change reduces page size by more than 10X and reduces page load time by 3-5X. As a result, the tree view can now load large DAGs that were causing 5XX error without the patch. List of optimizations applied to the view handler: * only seralize used task instance attributes to json instead of the whole ORM object * encode task instance attributes as array instead of dict * encode datetime in unix timestamp instead of iso formmat string * push task instance attribute construction into client side JS * remove redundant task instance attributes * simplify reduce_nodes() logic, remove unnecessary if statements * seralize JSON as string to be used with JSON.parse on the client side to speed up browser JS parse time * remove spaces in seralized JSON string to reduce payload size Co-Authored-By: QP Hou <[email protected]> (cherry-picked from c1c2d6a)
This change reduces page size by more than 10X and reduces page load time by 3-5X. As a result, the tree view can now load large DAGs that were causing 5XX error without the patch. List of optimizations applied to the view handler: * only seralize used task instance attributes to json instead of the whole ORM object * encode task instance attributes as array instead of dict * encode datetime in unix timestamp instead of iso formmat string * push task instance attribute construction into client side JS * remove redundant task instance attributes * simplify reduce_nodes() logic, remove unnecessary if statements * seralize JSON as string to be used with JSON.parse on the client side to speed up browser JS parse time * remove spaces in seralized JSON string to reduce payload size Co-Authored-By: QP Hou <[email protected]> (cherry-picked from c1c2d6a)
This change reduces page size by more than 10X and reduces page load time by 3-5X. As a result, the tree view can now load large DAGs that were causing 5XX errors before.
Another example: one of our DAGs' tree view had a page size of 200MB and took 1 minute to load. With this patch, it now loads within 24s with a page size of 17MB.
List of optimizations applied to the view handler:
whole ORM object
to speed up browser JS parse time
Issue link: AIRFLOW-6871
Make sure to mark the boxes below before creating PR: [x]
[AIRFLOW-NNNN]
. AIRFLOW-NNNN = JIRA ID** For document-only changes commit message can start with
[AIRFLOW-XXXX]
.In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.