Skip to content

AIRFLOW-1614 Replace inspect.stack() with sys._getframe()#2610

Closed
gbenison wants to merge 1 commit into
apache:masterfrom
gbenison:gcbenison2
Closed

AIRFLOW-1614 Replace inspect.stack() with sys._getframe()#2610
gbenison wants to merge 1 commit into
apache:masterfrom
gbenison:gcbenison2

Conversation

@gbenison

Copy link
Copy Markdown

inspect.stack() is really expensive, and slows down processing of dags
having large numbers (100s, 1000s) of subdags.

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

This patch simply swaps one method of getting the caller's file name for another. There should be no change in behavior (other than performance-wise) for either tests or production code.

inspect.stack() is really expensive, and slows down processing of dags
having large numbers (100s, 1000s) of subdags.
@codecov-io

codecov-io commented Sep 14, 2017

Copy link
Copy Markdown

Codecov Report

Merging #2610 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2610      +/-   ##
==========================================
- Coverage   70.72%   70.63%   -0.09%     
==========================================
  Files         150      150              
  Lines       11565    11564       -1     
==========================================
- Hits         8179     8168      -11     
- Misses       3386     3396      +10
Impacted Files Coverage Δ
airflow/models.py 87.02% <100%> (-0.01%) ⬇️
airflow/task_runner/bash_task_runner.py 93.33% <0%> (-6.67%) ⬇️
airflow/utils/helpers.py 53.44% <0%> (-2.88%) ⬇️
airflow/jobs.py 77.61% <0%> (-0.46%) ⬇️
airflow/executors/dask_executor.py 81.39% <0%> (+2.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9218a21...6729d07. Read the comment docs.

@saguziel

Copy link
Copy Markdown
Contributor

Curious, why is this faster? Is there any documentation on this? (Will it be more brittle because it's a private function?)

@gbenison

gbenison commented Sep 14, 2017 via email

Copy link
Copy Markdown
Author

@bolkedebruin

Copy link
Copy Markdown
Contributor

https://github.com/python/cpython/blob/a72d15c97f9837f6c9c6bd476a62175c942cc588/Lib/inspect.py#L1062

It isn't doing too much I would say. The difference probably is a pointer vs a method that calls several other methods.

LGTM

@saguziel

Copy link
Copy Markdown
Contributor

I think the difference is the first version may have to open the file for IO.

@saguziel

Copy link
Copy Markdown
Contributor

From some simple testing, it appears to not break anything.

@asfgit asfgit closed this in 558198e Sep 16, 2017
asfgit pushed a commit that referenced this pull request Sep 16, 2017
inspect.stack() is really expensive, and slows
down processing of dags
having large numbers (100s, 1000s) of subdags.

Closes #2610 from gbenison/gcbenison2

(cherry picked from commit 558198e)
Signed-off-by: Bolke de Bruin <bolke@xs4all.nl>
@gbenison
gbenison deleted the gcbenison2 branch September 22, 2017 23:18
ianburrell pushed a commit to ianburrell/airflow that referenced this pull request Jan 22, 2018
inspect.stack() is really expensive, and slows
down processing of dags
having large numbers (100s, 1000s) of subdags.

Closes apache#2610 from gbenison/gcbenison2
boralyl pushed a commit to upsight/airflow that referenced this pull request Apr 1, 2019
inspect.stack() is really expensive, and slows
down processing of dags
having large numbers (100s, 1000s) of subdags.

Closes apache#2610 from gbenison/gcbenison2
astahlman pushed a commit to lyft/airflow that referenced this pull request Jun 8, 2020
Adapted from this commit upstream: apache#2610:

This makes DagBag loading almost 3x faster on TARS staging.

- Before: 3m3s
- After: 1m9s

Original commit message:

    inspect.stack() is really expensive, and slows
    down processing of dags
    having large numbers (100s, 1000s) of subdags.
astahlman pushed a commit to lyft/airflow that referenced this pull request Jun 8, 2020
Adapted from this commit upstream: apache#2610:

This makes DagBag loading almost 3x faster on TARS staging.

- Before: 3m3s
- After: 1m9s

Original commit message:

    inspect.stack() is really expensive, and slows
    down processing of dags
    having large numbers (100s, 1000s) of subdags.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants