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

Perf: various optimizations #3602

Open
wants to merge 3 commits into
base: 7.x
Choose a base branch
from

Conversation

maartenbreddels
Copy link
Member

Gives a ~25-30% performance boost for creating widgets (benchmarking against 1000 buttons) by

  • not having comm a trait (no need for this)
  • doing less for every instance (and caching it per class) (.keys and .to_json lookup)
  • micro-optimization for state_get
    • not using unneeded _trait_to_json
    • also using the cached to_json (_trait_to_json_dict)
    • not checking not PY3 in loop
    • not checking drop_defaults in loop

Supersedes #3020

Benchmarks are done including this branch ipython/traitlets#777

comm is mostly for internal use, and only causes runtime overhead.
We were looking up traits and to_json at runtime for each call to
get_state.

We now use the cached ._trait_to_json_dict
@filipre
Copy link

filipre commented Feb 8, 2023

A somewhat wild guess but I think

not having comm a trait (no need for this)

might fix #3448. At least it would make debugging the issue a bit easier. What are the next steps for this PR?

@danielhollas
Copy link

Hi 👋

Just wanted to signal our interest in this PR. 😊 I've tested this branch and by my (very rough) benchmark, this would save us around 300 ms startup time for our app. 😮 (the linked traitlets PR already saved us 600ms, thanks so much @maartenbreddels for your work!)

Not sure if I can be of any help here besides testing. I've tried to merge with the current 7.x branch and there were only two minor conflicts that should be easily resolvable. I did not try to put this on top of 8.x branch.

@maartenbreddels
Copy link
Member Author

Just wanted to signal our interest in this PR. 😊

Thank you :)

I'm experimenting with this in solara, adding it to ipyvue at runtime widgetti/solara#389

I think the not having comm a trait feature might be something more for ipywidget9, as it might be a breaking change.

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.

None yet

3 participants