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

Speed up config.__getattr__ with a factor 10 corresponding to 5-10% app speed up. #5851

Merged
merged 11 commits into from
Nov 12, 2023

Conversation

MarcSkovMadsen
Copy link
Collaborator

@MarcSkovMadsen MarcSkovMadsen commented Nov 11, 2023

Closes #5850

Pytest

I've run the below test on the main branch and this branch.

from pathlib import Path
import pytest
from pyinstrument import Profiler
import panel as pn

TESTS_ROOT = Path.cwd()

@pytest.fixture(autouse=True)
def auto_profile(request):
    PROFILE_ROOT = (TESTS_ROOT / ".profiles")
    # Turn profiling on
    profiler = Profiler()
    profiler.start()

    yield  # Run test

    profiler.stop()
    PROFILE_ROOT.mkdir(exist_ok=True)
    results_file = PROFILE_ROOT / f"{request.node.name}.html"
    profiler.write_html(results_file)

item = pn.pane.Markdown("Hello World")

def method1():
    pn.config.loading_indicator
    pn.config.npm_cdn
    pn.config.sizing_mode
    pn.config.admin

def test_performance():
    N = 100000
    for i in range(N):
        method1()

I've achieved a 75% speed up. Left is main branch, Right is this branch

image

App

Compared to the profiling of the app in the reported issue, it looks much better. config.__getattribute__ is only in one place with a smaller proportion of 1% compared to the 11% before. Still I think that is too much.

image

Copy link

codecov bot commented Nov 11, 2023

Codecov Report

Merging #5851 (02d6795) into main (51adf14) will increase coverage by 11.54%.
Report is 6 commits behind head on main.
The diff coverage is 92.30%.

@@             Coverage Diff             @@
##             main    #5851       +/-   ##
===========================================
+ Coverage   72.36%   83.90%   +11.54%     
===========================================
  Files         290      290               
  Lines       42236    42264       +28     
===========================================
+ Hits        30562    35460     +4898     
+ Misses      11674     6804     -4870     
Flag Coverage Δ
ui-tests 40.90% <92.30%> (?)
unitexamples-tests 72.25% <92.30%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
panel/config.py 71.03% <92.30%> (+4.04%) ⬆️

... and 87 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@MarcSkovMadsen MarcSkovMadsen marked this pull request as ready for review November 11, 2023 09:30
@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Nov 11, 2023

I cannot do more now. The implementation is quite complicated. Maybe it should really be refactored? For example I don't understand why you want to ask again and again for the os.environ values. Why not set them once on startup?

I am not aware I've broken anything and the tests pass.

I hope you will review and finalize. This needs your knowledge @philippjfr . Thanks.

@MarcSkovMadsen MarcSkovMadsen changed the title Speed up config.__getattr__ Speed up config.__getattr__ with a factor 10 corresponding to 5% app speed up. Nov 11, 2023
@MarcSkovMadsen MarcSkovMadsen changed the title Speed up config.__getattr__ with a factor 10 corresponding to 5% app speed up. Speed up config.__getattr__ with a factor 10 corresponding to +5% app speed up. Nov 11, 2023
@philippjfr
Copy link
Member

Thanks for looking at this, refactoring config has been on my to-do list for a while. Will review the details and see if we can squeeze out a little more.

@MarcSkovMadsen MarcSkovMadsen changed the title Speed up config.__getattr__ with a factor 10 corresponding to +5% app speed up. Speed up config.__getattr__ with a factor 10 corresponding to 5-10% app speed up. Nov 11, 2023
@maximlt
Copy link
Member

maximlt commented Nov 12, 2023

Certainly config was the most difficult piece of code out there I had to deal with when working on Param 2.0.

@philippjfr philippjfr merged commit eb3ea19 into main Nov 12, 2023
@philippjfr philippjfr deleted the fix/config-get-attr branch November 12, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speed up initial load by 5-10% by making look up of config values efficient
3 participants